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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8527
+    return isKnownViaSimpleReasoning(Pred, S1, S2) ||
+           isImpliedCondOperandsHelper(Pred, S1, S2, OrigFoundLHS, FoundRHS);
   };
----------------
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).


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8571
+      auto *Num = getExistingSCEV(LL);
+
+      // Make sure that it exists and has the same type.
----------------
sanjoy wrote:
> This is minor, and I'll understand if you don't want to change it, but let's call `Num` `Numerator`.  `Num` is too ambiguous -- it can also mean `Number` for instance.
> 
> Paradoxically, I think `N` and `D` is less ambiguous than `Num` and `Denum`. :)
> 
> I'd also call `Denum` `Denom` if you must use an abbreviation, since the full spelling is `Denominator`.
Shame on me! :D Thanks for pointing out.


https://reviews.llvm.org/D30887