This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Prove implications for SCEVUnknown Phis
ClosedPublic

Authored by mkazantsev on Mar 2 2018, 3:34 AM.

Details

Summary

This patch teaches SCEV how to prove implications for SCEVUnknown nodes that are Phis.
If we need to prove Pred for LHS, RHS, and LHS is a Phi with possible incoming values
L1, L2, ..., LN, then if we prove Pred for (L1, RHS), (L2, RHS), ..., (LN, RHS) then we can also
prove it for (LHS, RHS). If both LHS and RHS are Phis from the same block, it is sufficient
to prove the predicate for values that come from the same predecessor block.

The typical case that it handles is that we sometimes need to prove that Phi(Len, Len - 1) >= 0
given that Len > 0. The new logic was added to isImpliedViaOperations and only uses it and
non-recursive reasoning to prove the facts we need, so it should not hurt compile time a lot.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn added a subscriber: fhahn.Mar 2 2018, 3:45 AM
mkazantsev planned changes to this revision.Mar 13 2018, 3:07 AM

New asserts failed on internal testing, I will investigate and update the patch accordingly.

mkazantsev added inline comments.Mar 13 2018, 11:54 PM
lib/Analysis/ScalarEvolution.cpp
9585 ↗(On Diff #136709)

This may fail if LHS is also a Phi from the same BB, but not a SCEVUnknown. It seems that it should be a check instead of assert.

Replaced the overconfident assertion with a check. It is something to follow-up on, but let's not do it in frames of this patch. Added a corresponding test.

anna added a subscriber: anna.Mar 26 2018, 1:58 PM

Comments inline. There's 2 dominator checks (dominates and isReachableFromEntry) that's done per phi operand. That seems like a good amount of compile time usage. There's also a isReachableFromEntry check within the dominates function. I cannot see a way to reduce these checks in your patch though.

lib/Analysis/ScalarEvolution.cpp
9586 ↗(On Diff #138498)

Pls separate out LPhi->getParent() into a var such as CommonParent and use here and below at line 9591.

9603 ↗(On Diff #138498)

I think you can extract out the logic here into another lambda because there's a lot of code duplication.
Really all we want to check using logic below and it can be extracted into isImplied(PhiNode *Phi, const Scev *LHSOrRHS).

Actually ignoring of unreachable predecessors was supposed to be an improvement, but we don't really need it. We can yield it for sake of compile time saving.

anna added a comment.Mar 28 2018, 5:41 AM

Actually ignoring of unreachable predecessors was supposed to be an improvement, but we don't really need it. We can yield it for sake of compile time saving.

Could you add a test case for unreachable predecessors? They are dominated by everything, so they would just pass through the checks below. I guess the expected improvement was to avoid cases where the unreachable blocks failed ProvedEasily check... A test case shows "unreachable blocks" go through the code, we just never handle them separately.

lib/Analysis/ScalarEvolution.cpp
9585 ↗(On Diff #136709)

I don't get this comment. We already finished checking that LHS is a phi in the above 2 cases in if-else ladder.

Will do.

lib/Analysis/ScalarEvolution.cpp
9585 ↗(On Diff #136709)

This comment relates to old version of this patch and is no longer relevant. :)

mkazantsev planned changes to this revision.Mar 29 2018, 4:23 AM
  1. Removed separate logic for unreachable blocks. They will now be handled as usual blocks because identifying them is more expensive in terms of compile time than the profit we expect from it.
  2. Removed redundant dominance check, limiting them to one essential check.
mkazantsev marked an inline comment as done.Mar 30 2018, 1:19 AM
mkazantsev added inline comments.
lib/Analysis/ScalarEvolution.cpp
9603 ↗(On Diff #138498)

Dominance check is only needed in one case, so I don't think it's a good idea to make such a lambda.

Adding IRCE tests for new cases (2 phis).

anna added inline comments.Apr 2 2018, 8:45 AM
lib/Analysis/ScalarEvolution.cpp
9583 ↗(On Diff #140392)

Could you pls give an idea of how these PendingMerges are used?

9615 ↗(On Diff #140392)

Nit: LBB == RPhi->getParent()

9618 ↗(On Diff #140392)

incoming

9633 ↗(On Diff #140392)

Can we also add an assert that the num of Incoming Values for LPhi is exactly 2?

Really just seems like RAR is a special case of a SCEV Phi with 2 incoming values. We just don't have a Phi node, it's recorded as an RAR.

9643 ↗(On Diff #140392)

Could you state here: LHS is phi and RHS is non-phi. It's obvious when looking at above code and the swapping, but easier when seeing this as a standalone third case.

mkazantsev marked 5 inline comments as done.Apr 2 2018, 11:53 PM
mkazantsev added inline comments.
lib/Analysis/ScalarEvolution.cpp
9643 ↗(On Diff #140392)

It is not utterly correct: RHS can be a Phi either SCEVUnknown or AddRec, but from different block than LPhi.

mkazantsev marked an inline comment as done.

Added comments and asserts, fixed typos.

anna added inline comments.Apr 3 2018, 6:59 AM
lib/Analysis/ScalarEvolution.cpp
9643 ↗(On Diff #140392)

ah you're right. It can be a phi from a different block, just that at the point of LBB, it will be exactly one value.

anna accepted this revision.Apr 3 2018, 6:59 AM

LGTM

This revision is now accepted and ready to land.Apr 3 2018, 6:59 AM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
9604

This assertion is triggering on the polly-AOSP buildbot: http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable/builds/477 .

I can try to reduce a testcase if you need it, but the assertion is pretty obviously just wrong: in general, SCEV doesn't care care whether a loop has a preheader.

mkazantsev added inline comments.Apr 6 2018, 12:37 AM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
9604

Thanks Eli, you're right, we don't actually need preheader here. I think this one should fix that: https://reviews.llvm.org/rL329379

efriedma added inline comments.Apr 6 2018, 12:52 PM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
9604

Hmm...

That's an improvement, but ScalarEvolution::createAddRecFromPHI is still much more generous than your assertions: it handles loops with multiple predecessors and multiple latches, at least in some cases.

mkazantsev added inline comments.Apr 6 2018, 11:00 PM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
9604

I don't think that in such loop we can have SCEVAddRec *in practice*, but after giving it some thought I don't see a theoretical problem with that: we have proved that all incoming values from all incoming blocks are the same, and all increment values from all latches are also the same, this may be happening... I will follow-up on this. At least now the buildbot does not crash with SCEV: http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable/builds/479