On Wed, Apr 12, 2023 at 10:55 PM Andrew MacLeod <[email protected]> wrote:
>
>
> On 4/12/23 07:01, Richard Biener wrote:
> > On Wed, Apr 12, 2023 at 12:59 PM Jakub Jelinek <[email protected]> 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;
> Andrew
>
> PS Of course, we still fail 101912. The only way I see us being
> able to do anything with that is to effectively peel the first iteration
> off, either physically, or logically with the path ranger to determine
> if a given use is actually reachable by the undefined value.
>
> <bb2>
>
> <bb 9> :
> # prevcorr_7 = PHI <prevcorr_19(D)(2), corr_22(8)>
> # leapcnt_8 = PHI <0(2), leapcnt_26(8)>
> if (leapcnt_8 < n_16) // 0 < n_16
> goto <bb 3>; [INV]
>
> <bb 3> :
> corr_22 = getint ();
> if (corr_22 <= 0)
> goto <bb 7>; [INV]
> else
> goto <bb 4>; [INV]
>
> <bb 4> :
> _1 = corr_22 == 1;
> _2 = leapcnt_8 != 0; // [0, 0] = 0 != 0
> _3 = _1 & _2; // [0, 0] = 0 & _2
> if (_3 != 0) // 4->5 is not taken on the path starting
> 2->9
> goto <bb 5>; [INV]
> else
> goto <bb 8>; [INV]
>
> <bb 5> : // We know this path is not taken when
> prevcorr_7 == prevcorr_19(D)(2)
> if (prevcorr_7 != 1)
> goto <bb 6>; [INV]
> else
> goto <bb 8>; [INV]
>
> <bb 6> :
> _5 = prevcorr_7 + -1;
> if (prevcorr_7 != 2)
> goto <bb 7>; [INV]
> else
> goto <bb 8>; [INV]
>
> Using the path ranger (Would it even need tweaks aldy?) , before issuing
> the warning the uninit code could easily start at each use, construct
> the path(s) to that use from the unitialized value, and determine when
> prevcorr is uninitialized, 2->9->3->4->5 will not be executed and of
> course,neither will 2->9->3->4->5->6
>
> I think threading already does something similar?
It does quite more convoluted things than that - it computes predicates on
paths with its own representation & simplifications. It might be worth
trying if replacing this with a lot of path rangers would help - but then
it heavily relies on relation simplification it implements (and at the
same time it does that very imperfectly).
Richard.
>
>
