[llvm] r228552 - Bugfix: ScalarEvolution incorrectly assumes that the start of certain

[Sanjoy Das]

Author: sanjoy
Date: Sun Feb  8 16:52:17 2015
New Revision: 228552

URL: http://llvm.org/viewvc/llvm-project?rev=228552&view=rev
Log:
Bugfix: ScalarEvolution incorrectly assumes that the start of certain
add recurrences don't overflow.

This change makes the optimization more restrictive.  It still assumes
that an overflowing `add nsw` is undefined behavior; and this change
will need revisiting once we have a consistent semantics for poison
values.

Differential Revision: http://reviews.llvm.org/D7331


Added:
    llvm/trunk/test/Analysis/ScalarEvolution/scev-prestart-nowrap.ll
Modified:
    llvm/trunk/lib/Analysis/ScalarEvolution.cpp

Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=228552&r1=228551&r2=228552&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
+++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sun Feb  8 16:52:17 2015
@@ -1364,7 +1364,24 @@ static const SCEV *getPreStartForSignExt
   const SCEVAddRecExpr *PreAR = dyn_cast<SCEVAddRecExpr>(
     SE->getAddRecExpr(PreStart, Step, L, SCEV::FlagAnyWrap));
 
-  if (PreAR && PreAR->getNoWrapFlags(SCEV::FlagNSW))
+  // WARNING: FIXME: the optimization below assumes that a sign-overflowing nsw
+  // operation is undefined behavior.  This is strictly more aggressive than the
+  // interpretation of nsw in other parts of LLVM (for instance, they may
+  // unconditionally hoist nsw arithmetic through control flow).  This logic
+  // needs to be revisited once we have a consistent semantics for poison
+  // values.
+  //
+  // "{S,+,X} is <nsw>" and "{S,+,X} is evaluated at least once" implies "S+X
+  // does not sign-overflow" (we'd have undefined behavior if it did).  If
+  // `L->getExitingBlock() == L->getLoopLatch()` then `PreAR` (= {S,+,X}<nsw>)
+  // is evaluated every-time `AR` (= {S+X,+,X}) is evaluated, and hence within
+  // `AR` we are safe to assume that "S+X" will not sign-overflow.
+  //
+
+  BasicBlock *ExitingBlock = L->getExitingBlock();
+  BasicBlock *LatchBlock = L->getLoopLatch();
+  if (PreAR && PreAR->getNoWrapFlags(SCEV::FlagNSW) &&
+      ExitingBlock != nullptr && ExitingBlock == LatchBlock)
     return PreStart;
 
   // 2. Direct overflow check on the step operation's expression.

Added: llvm/trunk/test/Analysis/ScalarEvolution/scev-prestart-nowrap.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/ScalarEvolution/scev-prestart-nowrap.ll?rev=228552&view=auto
==============================================================================
--- llvm/trunk/test/Analysis/ScalarEvolution/scev-prestart-nowrap.ll (added)
+++ llvm/trunk/test/Analysis/ScalarEvolution/scev-prestart-nowrap.ll Sun Feb  8 16:52:17 2015
@@ -0,0 +1,116 @@
+; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s
+
+; An example run where SCEV(%postinc)->getStart() may overflow:
+;
+; %start = INT_SMAX
+; %low.limit = INT_SMIN
+; %high.limit = < not used >
+;
+; >> entry:
+;  %postinc.start = INT_SMIN
+;
+; >> loop:
+;  %idx = %start 
+;  %postinc = INT_SMIN
+;  %postinc.inc = INT_SMIN + 1
+;  %postinc.sext = sext(INT_SMIN) = i64 INT32_SMIN
+;  %break.early = INT_SMIN `slt` INT_SMIN = false
+;  br i1 false, ___,  %early.exit
+;
+; >> early.exit:
+;  ret i64 INT32_SMIN
+
+
+define i64 @bad.0(i32 %start, i32 %low.limit, i32 %high.limit) {
+; CHECK-LABEL: Classifying expressions for: @bad.0
+ entry:
+  %postinc.start = add i32 %start, 1
+  br label %loop
+
+ loop:
+  %idx = phi i32 [ %start, %entry ], [ %idx.inc, %continue ]
+  %postinc = phi i32 [ %postinc.start, %entry ], [ %postinc.inc, %continue ]
+  %postinc.inc = add nsw i32 %postinc, 1
+  %postinc.sext = sext i32 %postinc to i64
+; CHECK:  %postinc.sext = sext i32 %postinc to i64
+; CHECK-NEXT:  -->  {(sext i32 (1 + %start) to i64),+,1}<nsw><%loop>
+  %break.early = icmp slt i32 %postinc, %low.limit
+  br i1 %break.early, label %continue, label %early.exit
+
+ continue:
+  %idx.inc = add nsw i32 %idx, 1
+  %cmp = icmp slt i32 %idx.inc, %high.limit
+  br i1 %cmp, label %loop, label %exit
+
+ exit:
+  ret i64 0
+
+ early.exit:
+  ret i64 %postinc.sext
+}
+
+define i64 @bad.1(i32 %start, i32 %low.limit, i32 %high.limit, i1* %unknown) {
+; CHECK-LABEL: Classifying expressions for: @bad.1
+ entry:
+  %postinc.start = add i32 %start, 1
+  br label %loop
+
+ loop:
+  %idx = phi i32 [ %start, %entry ], [ %idx.inc, %continue ], [ %idx.inc, %continue.1 ]
+  %postinc = phi i32 [ %postinc.start, %entry ], [ %postinc.inc, %continue ], [ %postinc.inc, %continue.1 ]
+  %postinc.inc = add nsw i32 %postinc, 1
+  %postinc.sext = sext i32 %postinc to i64
+; CHECK:  %postinc.sext = sext i32 %postinc to i64
+; CHECK-NEXT:  -->  {(sext i32 (1 + %start) to i64),+,1}<nsw><%loop>
+  %break.early = icmp slt i32 %postinc, %low.limit
+  br i1 %break.early, label %continue.1, label %early.exit
+
+ continue.1:
+  %cond = load volatile i1* %unknown
+  %idx.inc = add nsw i32 %idx, 1
+  br i1 %cond, label %loop, label %continue
+
+ continue:
+  %cmp = icmp slt i32 %idx.inc, %high.limit
+  br i1 %cmp, label %loop, label %exit
+
+ exit:
+  ret i64 0
+
+ early.exit:
+  ret i64 %postinc.sext
+}
+
+
+; WARNING: FIXME: it is safe to make the inference demonstrated here
+; only if we assume `add nsw` has undefined behavior if the result
+; sign-overflows; and this interpretation is stronger than what most
+; of LLVM assumes.  This test here only serves as a documentation of
+; current behavior and will need to be revisited once we've decided
+; upon a consistent semantics for nsw (and nuw) arithetic operations.
+;
+define i64 @good(i32 %start, i32 %low.limit, i32 %high.limit) {
+; CHECK-LABEL: Classifying expressions for: @good
+ entry:
+  %postinc.start = add i32 %start, 1
+  br label %loop
+
+ loop:
+  %idx = phi i32 [ %start, %entry ], [ %idx.inc, %loop ]
+  %postinc = phi i32 [ %postinc.start, %entry ], [ %postinc.inc, %loop ]
+  %postinc.inc = add nsw i32 %postinc, 1
+  %postinc.sext = sext i32 %postinc to i64
+; CHECK: %postinc.sext = sext i32 %postinc to i64
+; CHECK-NEXT: -->  {(1 + (sext i32 %start to i64)),+,1}<nsw><%loop>
+
+  %break.early = icmp slt i32 %postinc, %low.limit
+  %idx.inc = add nsw i32 %idx, 1
+  %cmp = icmp slt i32 %idx.inc, %high.limit
+  br i1 %cmp, label %loop, label %exit
+
+ exit:
+  ret i64 0
+
+ early.exit:
+  ret i64 %postinc.sext
+}