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

[Philip Reames via Phabricator via llvm-commits]

reames updated this revision to Diff 369813.
reames retitled this revision from "[SCEV] Allow negative steps for LT exit count computation" to "[SCEV] Allow negative steps for LT exit count computation for unsigned comparisons".
reames added a comment.

Narrowing focus to unsigned comparisons.

The reasoning about signed cases makes my head hurt, and even after staring at it for a while, I'm neither sure the code is correct or incorrect for signed comparisons.  Give my actual interest is unsigned IVs, I propose we just defer the signed cases until someone else has time, interest, and the ability to reason through it.

@efriedma Sorry for the silence on this for so long.

p.s. I'm going to mark this as dependent on D109029 <https://reviews.llvm.org/D109029> as the diff is built on top of that patch and might be confusing otherwise, but there's no semantic connection between the two.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104140/new/

https://reviews.llvm.org/D104140

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


Index: llvm/test/Analysis/ScalarEvolution/trip-count-negative-stride.ll
===================================================================
--- llvm/test/Analysis/ScalarEvolution/trip-count-negative-stride.ll
+++ llvm/test/Analysis/ScalarEvolution/trip-count-negative-stride.ll
@@ -85,8 +85,8 @@
 }
 
 ; CHECK-LABEL: Determining loop execution counts for: @ult_129_unknown_start
-; CHECK: Loop %for.body: Unpredictable backedge-taken count
-; CHECK: Loop %for.body: Unpredictable max backedge-taken count
+; CHECK: Loop %for.body: backedge-taken count is (((127 + (-1 * (1 umin (127 + (-1 * %start) + (-128 umax (-127 + %start)))))<nuw><nsw> + (-1 * %start) + (-128 umax (-127 + %start))) /u -127) + (1 umin (127 + (-1 * %start) + (-128 umax (-127 + %start)))))
+; CHECK: Loop %for.body: max backedge-taken count is 1
 
 define void @ult_129_unknown_start(i8 %start) mustprogress {
 entry:
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===================================================================
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -11534,6 +11534,12 @@
   if (IsSigned && BitWidth == 1)
     return getZero(Stride->getType());
 
+  // This code has only been closely audited for negative strides in the
+  // unsigned comparison case, it may be correct for signed comparison, but
+  // that needs to be established.
+  assert((!IsSigned || !isKnownNonPositive(Stride)) &&
+         "Stride is expected strictly positive for signed case!");
+
   // Calculate the maximum backedge count based on the range of values
   // permitted by Start, End, and Stride.
   APInt MinStart =
@@ -11640,20 +11646,15 @@
     // 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) || !loopHasNoAbnormalExits(L))
+    if (PredicatedIV || !NoWrap || !loopIsFiniteByAssumption(L) ||
+        !loopHasNoAbnormalExits(L))
+      return getCouldNotCompute();
+
+    // This bailout is protecting the logic in computeMaxBECountForLT which
+    // has not yet been sufficiently auditted or tested with negative strides.
+    // We used to filter out all known-non-positive cases here, we're in the
+    // process of being less restrictive bit by bit.
+    if (IsSigned && isKnownNonPositive(Stride))
       return getCouldNotCompute();
 
     if (!isKnownNonZero(Stride)) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104140.369813.patch
Type: text/x-patch
Size: 3134 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210831/b9618a3b/attachment.bin>