[PATCH] D31238: [ScalarEvolution] Re-enable 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.

lgtm with nits inline.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:8652
+      // cache as SCEVCouldNotCompute to avoid the infinite recursion. This is a
+      // sad thing. To avoid this, we only want to create SCEVs that are
+      // constants in this section. So we bail if Denominator is not a constant.
----------------
Minor, but I'd drop the "This is a sad thing." -- I don't think that observation adds anything here. :)


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8663
+
+      // Make sure that it exists and has the same type.
+      if (!Numerator || Numerator->getType() != FoundLHS->getType())
----------------
I'd drop this "Make sure ..." comment and also the one below -- the code is descriptive enough to tell you what you're doing and the comment does not add much.

If possible add a line on _why_ you care about these things, but if that's going to be too verbose, then I'd say just remove this comment and the one below.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8667
+
+      // Make sure that the numerator matches with FoundLHs and the denominator
+      // is positive.
----------------
s/FoundLHs/FoundLHS/


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8676
+        // One of types is a pointer and another one is not. We cannot extend
+        // them properly to a wider type, so let us just reject this case.
+        return false;
----------------
The correct fix here is to use `getEffectiveSCEVType` for `DTy`, `FRHSTy` etc. instead of just calling `getType()` on the `Value`.

However, for a first pass what you have here is fine.  But please add a TODO.


https://reviews.llvm.org/D31238