Re: PR61385: phiopt drops some PHIs

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 4, 2014 at 9:59 AM, Jeff Law <law@redhat.com> wrote:
> On 06/04/14 01:46, Marc Glisse wrote:
>>
>> On Tue, 3 Jun 2014, Jeff Law wrote:
>>
>>> On 06/03/14 08:08, Richard Biener wrote:
>>>>
>>>> All arguments get the same value (and the PHI in middle-bb is surely
>>>> a singleton?), so it's way better to re-materialize the PHI as a
>>>> gimple assignment at the start of the basic block.  If they are
>>>> singletons (which I expect), the easiest way is to propagate their
>>>> single argument into all uses and simply remove them.
>>
>>
>> I was all for propagating, but replace_uses_by calls fold_stmt on the
>> using statements (that's normal). If I first move the statement and then
>> replace, I may call fold_stmt on a stmt that isn't dominated by the defs
>> of all its arguments. If I first replace, the statement I am supposed to
>> move may have been modified so I need to find out what happened to it.
>> In the end, the assignment seems easier and safer (and not too bad since
>> this case almost never happens in practice).
>>
>>> We certainly want to get rid of them :-)
>>>
>>> I'd start by finding out which pass left the degenerate, ensure it's
>>> not marked as SSA_NAME_OCCURS_IN_ABNORMAL_PHI, then propagate it away.
>>>
>>> If it's a systemic problem that a particular pass can leave degenerate
>>> PHIs, then you might want to schedule the phi-only propagator to run
>>> after that pass.  It does const/copy propagation seeding from
>>> degenerate PHIs, so it ought to be very fast.
>>
>>
>> This one comes from VRP2 apparently. But it isn't the only pass that
>> produces singleton phis. I didn't look at them closely, but I assume LIM
>> is normal, DOM maybe less so, etc.
>
> DOM & VRP can create them via jump threading.  Right now we only fire up the
> pass_phi_only_cprop after DOM though.

Right, if we want to get rid of the degenerates early we have to run
pass_phi_only_cprop after VRP as well.

> DOM runs not too long after LIM and it will propagate away the degenerates
> as well.

Not sure where LIM would create degenerate PHIs, but I guess you
simply see loop-closed PHIs here which are degenerate.

> One could argue that propagating away the degenerates ought to occur
> immediately before any phi-opt pass.

I'd say we should run phi_only_cprop after the first VRPs instead and
move the 2nd VRP before the existing phi_only_cprop pass.  I'll do
the appropriate change.

>>
>> bootstrap+testsuite on x86_64-linux-gnu as usual.
>>
>> 2014-06-04  Marc Glisse  <marc.glisse@inria.fr>
>>
>>      PR tree-optimization/61385
>> gcc/
>>      * tree-ssa-phiopt.c (value_replacement): Replace singleton PHIs
>>      with assignments.
>> gcc/testsuite/
>>      * gcc.dg/tree-ssa/pr61385.c: New file.
>
> Don't you have to verify neither the source nor the destination are marked
> with SSA_NAME_OCCURS_IN_ABNORMAL_PHI?  And if so, you can't propagate away
> the PHI (which may mean we need to avoid the optimization in that case).
>
> I also believe that when turning a PHI into an assignment  you have to
> preserve the parallel nature of the PHI.  If the result of one PHI is used
> as an input in another PHI, then you have to work harder (see the out-of-ssa
> code).  Perhaps this can never be the case with a degenerate.

Hmm, indeed that can happen.

So I'd say we should instead simply bail out if the middle-bb has a PHI node.

Simple and safe.  And with the pass ordering fixes above we shouldn't see
degenerates anyway.

Richard.

> Jeff