Re: [PATCH] PR tree-optimization/109462 - Don't use ANY PHI equivalences in range-on-entry.

[Richard Biener <richard.guenther@gmail.com>]

> Am 13.04.2023 um 17:49 schrieb Andrew MacLeod <amacleod@redhat.com>:
> 
> 
>> On 4/13/23 09:56, Richard Biener wrote:
>>> On Wed, Apr 12, 2023 at 10:55 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>>> 
>>> On 4/12/23 07:01, Richard Biener wrote:
>>>> On Wed, Apr 12, 2023 at 12:59 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> Would be nice.
>>>>> 
>>>>> Though, I'm afraid it still wouldn't fix the PR101912 testcase, because
>>>>> it has exactly what happens in this PR, undefined phi arg from the
>>>>> pre-header and uses of the previous iteration's value (i.e. across
>>>>> backedge).
>>>> Well yes, that's what's not allowed.  So when the PHI dominates the
>>>> to-be-equivalenced argument edge src then the equivalence isn't
>>>> valid because there's a place (that very source block for example) a use of the
>>>> PHI lhs could appear and where we'd then mixup iterations.
>>>> 
>>> If we want to implement this cleaner, then as you say, we don't create
>>> the equivalence if the PHI node dominates the argument edge.  The
>>> attached patch does just that, removing the both the "fix" for 108139
>>> and the just committed one for 109462, replacing them with catching this
>>> at the time of equivalence registering.
>>> 
>>> It bootstraps and passes all regressions tests.
>>> Do you want me to check this into trunk?
>> Uh, it looks a bit convoluted.  Wouldn't the following be enough?  OK
>> if that works
>> (or fixed if I messed up trivially)
>> 
>> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
>> index e81f6b3699e..9c29012e160 100644
>> --- a/gcc/gimple-range-fold.cc
>> +++ b/gcc/gimple-range-fold.cc
>> @@ -776,7 +776,11 @@ fold_using_range::range_of_phi (vrange &r, gphi
>> *phi, fur_source &src)
>>           if (!seen_arg)
>>             {
>>               seen_arg = true;
>> -             single_arg = arg;
>> +             // Avoid registering an equivalence if the PHI dominates the
>> +             // argument edge.  See PR 108139/109462.
>> +             if (dom_info_available_p (CDI_DOMINATORS)
>> +                 && !dominated_by_p (CDI_DOMINATORS, e->src, gimple_bb (phi)))
>> +               single_arg = arg;
>>             }
>>           else if (single_arg != arg)
>>             single_arg = NULL_TREE;
>> 
>> 
> It would exposes a slight hole.. in cases where there is more than one copy of the name, ie:
> 
> for a_2 = PHI <c_3, c_3>  we currently will create an equivalence between  a_2 and c_3 because its considered a single argument.  Not a big deal for this case since all arguments are c_3, but the hole would be when we have something like:
> 
> a_2 = PHI<c_3, c_3, d_4(D)>    if d_4 is undefined, then with the above patch we would only check the dominance of the first edge with c_3. we'd need to check all of them.

I didn’t think so, but on a second thought for multiple backedges and an irreducible region you are right.  Once we see an edge with the reverse domination we’re fine though.  I originally checked all edges but thought checking the first is enough.

> The patch is slightly convoluted because we always defer checking the edge/processing single arguments until we think there is a reason to (for performance).  My patch simple does the deferred check on the previous edge and sets the new one so that we would check both edges are valid before setting the equivalence.  Even as it is with this deferred check we're about 0.4% slower in VRP. IF we didnt do this deferring, then every PHI is going to have a check.
> 
> And along the way, remove the boolean seen_arg because having single_arg_edge set produces the same information.
> 
> Perhaps it would be cleaner to simply defer the entire thing to the end, like so.
> Performance is pretty much identical in the end.
> 
> Bootstraps on x86_64-pc-linux-gnu, regressions are running. Assuming no regressions pop up,   OK for trunk?
> 
> Andrew
> 
> 
> 
> 
> 
> <462c.patch>