[PATCH] D31238: [ScalarEvolution] Re-enable Predicate implication from operations

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8549
+                                             unsigned Depth) {
+  // We want to avoid hurting the compile time with analysis of too big trees.
+  if (Depth > MaxSCEVOperationsImplicationDepth)
----------------
mkazantsev wrote:
> sanjoy wrote:
> > How about asserting here that
> > 
> >  - The types of `LHS` and `RHS` match
> >  - The types of `FoundLHS` and `FoundRHS` match
> > 
> > ?
> This assertion actually fails on test Transforms/IndVarSimplify/2011-11-01-lftrptr.ll even if placed to the beginning of isImpliedCondOperandsHelper method (and ViaOperations stuff is disabled). I am not certain for 100% is it a bug or not, but potentially it is. Even if so, its reason lies outside this patch. I will try to investigate it.
> 
> Currently we do not require the match of these types for our code here (yet I assumed that they do match). All required type conversions within isImpliedViaOperations are done explicitly. So let us not do it here. Instead, I will try to understand whether the failure on this assertion on those test is a bug, and if it is, I will add them with the fix patch.
The reason of this failure is that this kind of assertions is overprotective. We should check only type size match to allow comparisons between integers and pointers.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8589
+    // TODO: Maybe try to get RHS from sext to catch more cases?
+    if (LHSAddExpr->getType() != RHS->getType())
+      return false;
----------------
mkazantsev wrote:
> This is not entirely correct, because the type lf ADD is defined by the type of its last argument. It may not match with the type of the 1st argument. The problem is that one of the operands is sometimes a reference.
> 
>  So we run into "assert(S1->getType() == S2->getType() && "Proving for wrong types?");" failure in this case.
Actually this is valid, but the assert is overprotective. We should not check the types match, only width match.


https://reviews.llvm.org/D31238