[PATCH] D12073: Make ScalarEvolution::isKnownPredicate a little smarter

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

In http://reviews.llvm.org/D12073#226151, @hfinkel wrote:

>




> There are now three failing tests, mostly due to IV widening that now does not happen. The new known-predicate results are:

> 

> test/Transforms/IndVarSimplify/elim-extend.ll:

> 

> - {%init,+,1}<nsw><%loop> sle {(1 + %init),+,1}<nsw><%loop>

> - {(sext i32 %init to i64),+,1}<nsw><%loop> sle {(1 + (sext i32 %init to i64)),+,1}<nsw><%loop>

> - {-1,+,1}<nsw><%outerloop> sle {1,+,1}<nuw><%outerloop>

> - {-1,+,1}<nsw><%outerloop> sle {1,+,1}<nuw><nsw><%outerloop>

> 

>   test/Transforms/IndVarSimplify/iv-sext.ll:

> - {-1,+,1}<nsw><%bb> sle {1,+,1}<nuw><%bb>

> - {-1,+,1}<nsw><%bb> sle {1,+,1}<nuw><nsw><%bb>

> 

>   test/Transforms/IndVarSimplify/strengthen-overflow.ll:

> - {%init,+,1}<nsw><%loop> sle {(1 + %init),+,1}<nsw><%loop>

> 

>   These look right, but some consumer of the information seems to make a worse no-wrap deductions as a result (thus, this patch is still WIP).


I suspect (haven't verified) this is due to SCEV's caching mechanism. You end up creating a "Sext(LHS)" too soon, before proving all of the things needed to show that LHS is nsw.  Then this suboptimal result gets cached.

In other words, I suspect the stack trace looks like this (youngest frame at top) initially:

  == false because of PendingLoopPredicates, set in frame (A)
  getImpliedCond(...)  // ends up looking at VAL
  getLoopBackedgeCount(...)
  getSignExtend(VAL)  // due to your change
  getImpliedCond(...)  // ends up looking at VAL .. (A)
  getLoopBackedgeCount(...)
  getSignExtend(VAL)

This goes on as

    getSignExtend(VAL)  ==> no simplification, and we end up with
          UniqueSCEVs[(SignExtend, VAL)] = SignExtendExpr VAL, and
  	does not set the <nsw> bit in VAL
    getImpliedCond(...)  // ends up looking at VAL .. (A)
    getLoopBackedgeCount(...)
    getSignExtend(VAL)

And then finally the oldest frame returns:

  getSignExtend(VAL)  // this returns a widened version of VAL (instead of
    an explicit sign extend), and also set VAL's nsw bit

However, the damage has been done since `UniqueSCEVs[(SignExtend, VAL)] == (SignExtendExpr VAL)` and that's the value `ScalarEvolution::getSignExtendExpr`  will return from now on.

If you implement your check without actually creating a sign extend, but by directly inferring no-overflow from the nsw or nuw flags, then I suspect this won't happen.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:7355
@@ +7354,3 @@
+  case ICmpInst::ICMP_SLT:
+    if (SE.isKnownNegative(SignedDiff()))
+      return true;
----------------
I think this change would be cleaner if `SignedDiff` took explicit `LHS` and `RHS` arguments.


http://reviews.llvm.org/D12073