[PATCH] D104140: [SCEV] Allow negative steps for LT exit count computation

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: nikic, mkazantsev, efriedma.
Herald added subscribers: javed.absar, bollu, hiraditya, mcrosier.
reames requested review of this revision.
Herald added a project: LLVM.

This bit of code is incredibly suspicious.  It allows fully unknown (but potentially negative) steps, but not steps known to be negative.  The comment about scev flag inference is worrying, but also not correct to my knowledge.

At best, this might be covering up some related miscompile.  However, there's no test in tree for it, the review history doesn't include obvious motivation, and the C++ example doesn't appear to give wrong results when hand translated to IR.  I think it's time to remove this and see what falls out.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104140

Files:
  llvm/lib/Analysis/ScalarEvolution.cpp
  llvm/test/Analysis/ScalarEvolution/trip-count-unknown-stride.ll


Index: llvm/test/Analysis/ScalarEvolution/trip-count-unknown-stride.ll
===================================================================
--- llvm/test/Analysis/ScalarEvolution/trip-count-unknown-stride.ll
+++ llvm/test/Analysis/ScalarEvolution/trip-count-unknown-stride.ll
@@ -106,5 +106,45 @@
   ret void
 }
 
+; In this example, we branch on poison on the first iteration, so the
+; loop is undefined, and thus any answer is valid.
+; CHECK: Determining loop execution counts for: @foo5
+; CHECK: Loop %for.body: backedge-taken count is 1
+; CHECK: Loop %for.body: max backedge-taken count is 1
+
+define void @foo5(i32* nocapture %A, i32 %n, i32 %s) {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %entry, %for.body
+  %i.05 = phi i8 [ %add, %for.body ], [ 127, %entry ]
+  %add = add nsw nuw i8 %i.05, 129
+  %cmp = icmp ult i8 %add, 128
+  br i1 %cmp, label %for.body, label %for.end, !llvm.loop !8
+
+for.end:                                          ; preds = %for.body, %entry
+  ret void
+}
+
+; Defined version of foo5, in this case 1 is the correct answer
+; CHECK: Determining loop execution counts for: @foo6
+; CHECK: Loop %for.body: backedge-taken count is 1
+; CHECK: Loop %for.body: max backedge-taken count is 1
+
+define void @foo6(i32* nocapture %A, i32 %n, i32 %s) {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %entry, %for.body
+  %i.05 = phi i8 [ %add, %for.body ], [ 127, %entry ]
+  %add = add nsw nuw i8 %i.05, 129
+  %cmp = icmp ult i8 %add, 128
+  br i1 %cmp, label %for.body, label %for.end, !llvm.loop !8
+
+for.end:                                          ; preds = %for.body, %entry
+  ret void
+}
+
+
 !8 = distinct !{!8, !9}
 !9 = !{!"llvm.loop.mustprogress"}
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===================================================================
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -11459,20 +11459,7 @@
     // The positive stride case is the same as isKnownPositive(Stride) returning
     // true (original behavior of the function).
     //
-    // We want to make sure that the stride is truly unknown as there are edge
-    // cases where ScalarEvolution propagates no wrap flags to the
-    // post-increment/decrement IV even though the increment/decrement operation
-    // itself is wrapping. The computed backedge taken count may be wrong in
-    // such cases. This is prevented by checking that the stride is not known to
-    // be either positive or non-positive. For example, no wrap flags are
-    // propagated to the post-increment IV of this loop with a trip count of 2 -
-    //
-    // unsigned char i;
-    // for(i=127; i<128; i+=129)
-    //   A[i] = i;
-    //
-    if (PredicatedIV || !NoWrap || isKnownNonPositive(Stride) ||
-        !loopIsFiniteByAssumption(L))
+    if (PredicatedIV || !NoWrap || !loopIsFiniteByAssumption(L))
       return getCouldNotCompute();
   } else if (!Stride->isOne() && !NoWrap) {
     auto isUBOnWrap = [&]() {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104140.351513.patch
Type: text/x-patch
Size: 3080 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210611/9abf328d/attachment.bin>