[PATCH] D73181: [SCEV] Do not use backedge SCEV of PHI if its input is another PHI
Denis Antrushin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 23 08:19:47 PST 2020
dantrushin marked an inline comment as done.
dantrushin added inline comments.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8247
+ if (!isa<PHINode>(BackedgeVal) &&
+ IsAvailableOnEntry(LI, DT, OnBackedge, PN->getParent()))
return OnBackedge;
----------------
sanjoy.google wrote:
> dantrushin wrote:
> > sanjoy.google wrote:
> > > dantrushin wrote:
> > > > sanjoy.google wrote:
> > > > > dantrushin wrote:
> > > > > > Bailing out here when any PHI is seen, because I did not find convinient API to get SCEV at previous iteration.
> > > > > > Is there one?
> > > > > I don't think you need to go through SCEV here. You can just pick the `llvm::Value` for the PHI for the backedge and get the SCEV for that.
> > > > Excuse me, I don't understand (or, rather my comment was unclear).
> > > >
> > > > Existing code already does that. In terms of test case below, `PN` is PHI `%local_3_4`
> > > > Its backedge `Value` is `%local_6_6` which is another PHI from the same block.
> > > > But using `%local_6_6`'s SCEV as value for `%local_3_4` is wrong, because `%local_3_4` uses value of `%local_6_6`
> > > > from previous iteration
> > > > So my fix is not to perform this optimization when PHI's input is another PHI
> > > >
> > > > Or you mean something else?
> > > I was unclear as well.
> > >
> > > I'm suggesting we do this:
> > >
> > > ```
> > > Value *BackedgeVal = PN->getIncomingValue(InLoopPred);
> > > if (auto* InputPN = dyn_cast<PHINode>(BackedgeVal)) {
> > > if (InputPN->getParent() == PN->getBlock()) {
> > > // PHI translation
> > > BackedgeVal = InputPN->getIncomingValue(InLoopPred);
> > > }
> > > }
> > > const SCEV *OnBackedge = getSCEV(BackedgeVal);
> > > // ... no other change
> > > ```
> > That gave me even "wronger" answer :)
> > Correct answer for the test is -276.
> > Current LLVM produces -277
> > With your suggestion I get -278.
> >
> I see. I now think that the problem is not with using a PHI node, but with any loop varying value. IOW if you have:
>
> ```
> loop:
> A = PHI(0, B)
> B = SomethingWhoseValueChangesOnEachIteration();
> ```
>
> then the exit value of `A` is *not* `B`, even if the loop body is guaranteed to execute more than once. The exit value of `A` is the value of `B` in the one-beofore-last iteration, not `B` in the last iteration.
>
> So even with your change, if I modify your unit test to have `%local_3_4 = phi i32 [ 2, %entry ], [ %5, %latch ]` then we will run into the same problem even with your change.
>
> Does that sound right?
Ugh. You're right.
It looks like original comment was correct about loop invariant, it's just D63224 missed correct check for that...
I'll rework the patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73181/new/
https://reviews.llvm.org/D73181
More information about the llvm-commits
mailing list