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

[Richard Biener via Gcc-patches]

> Am 13.04.2023 um 17:49 schrieb Andrew MacLeod <amacl...@redhat.com>:
> 
> 
>> On 4/13/23 09:56, Richard Biener wrote:
>>> On Wed, Apr 12, 2023 at 10:55 PM Andrew MacLeod <amacl...@redhat.com> wrote:
>>> 
>>> On 4/12/23 07:01, Richard Biener wrote:
>>>> On Wed, Apr 12, 2023 at 12:59 PM Jakub Jelinek <ja...@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.
> 
> 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?

Yes.  I like this more.  OK

Richard 

> Andrew
> 
> 
> 
> 
> 
> <462c.patch>