[PATCH] D30887: [ScalarEvolution] Predicate implication from operations

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

This looks good to me.

I have two concerns that I've mentioned inline.  Feel free to fix them however you see fit.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8527
+    return isKnownViaSimpleReasoning(Pred, S1, S2) ||
+           isImpliedCondOperandsHelper(Pred, S1, S2, OrigFoundLHS, FoundRHS);
   };
----------------
mkazantsev wrote:
> sanjoy wrote:
> > Can we avoid the recursion via `isImpliedCondOperandsHelper`?
> That's the point of the optimization! Sometimes we cannot simply prove that (a + b > c), but can do it via the context passed from division. And vice versa, if we have something like ( a / ( a / b + c)), we can prove the inner division using the context from outer division.
> 
> We are now not creating non-constant SCEVs, so all recursion will stay between the isImpliedCondOperandsHelper and isImpliedViaOperations, and we will always go down the syntax tree and never go into the infinite recursion. Actually its depth is not really big (not bigger than the depth of expressions).
Do you rely only recursing into `isImpliedViaOperations` or into the whole of `isImpliedCondOperandsHelper`?  If the former, I'd be much more comfortable if you:

 - Changed `IsProvedViaContext` to directly call into isImpliedViaOperations
 - Passed along a `Depth` parameter and cap it at a fairly low threshold (let's say 3?) to protect us from the truly pathological cases

We should implement the second point even if we need to recurse into `isImpliedCondOperandsHelper`, but if recursing into `isImpliedViaOperations` directly gives us what we want for cheap, we should just do that.

I'm not just worried about infinite recursion -- `isImpliedCondOperandsHelper` is called often enough that even a somewhat deep recursion here will slow things down.


================
Comment at: test/Analysis/ScalarEvolution/scev-division.ll:8
+; CHECK:         Determining loop execution counts for: @test01
+; CHECK:         Loop %header: Predicated backedge-taken count is (-1 + %n.div.2)<nsw>
+entry:
----------------
Are you intentionally matching for `Predicated backedge-taken count`?  I'd have expected you to match for just `Loop %xxx: backedge-taken count is yyy` etc.


https://reviews.llvm.org/D30887