[PATCH] D108921: [SCEV] If max BTC is zero, then so is the exact BTC

[Philip Reames via Phabricator via llvm-commits]

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

The subtle bit is explaining why the two codepaths have a difference while both are correct.  The test case with modifications is a good example, so let's discuss in terms of it.

- The previous exact bounds for this example of (-126 + (126 smax %n))<nsw> can evaluate to either 0 or 1.  Both are "correct" results, but only one of them results in a well defined loop.  If %n were 127 (the only possible value producing a trip count of 1), then the loop must execute undefined behavior.  As a result, we can ignore the TC computed when %n is 127.  All other values produce 0.
- The max taken count computation uses the limit (i.e. the maximum value END can be without resulting in UB) to restrict the bound computation.  As a result, it returns 0 which is also correct.

WARNING: The logic above only holds for a single exit loop.  The current upper bound logic is as far as I can tell incorrect for multiple exits.  I'm going to fix that in a separate patch and make this one dependent on it once posted.

An alternate approach here would be to add the limit logic to the symbolic path.  I haven't played with this extensively, but I'm hesitant because a) the term is optional and b) I'm not sure it'll reliably simplify away.  As such, the resulting code quality from expansion might actually get worse.

This was noticed while trying to figure out why D108848 <https://reviews.llvm.org/D108848> wasn't NFC, but is otherwise standalone.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108921

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


Index: llvm/test/Analysis/ScalarEvolution/max-trip-count.ll
===================================================================
--- llvm/test/Analysis/ScalarEvolution/max-trip-count.ll
+++ llvm/test/Analysis/ScalarEvolution/max-trip-count.ll
@@ -458,7 +458,7 @@
 
 define void @max_overflow(i8 %n) mustprogress {
 ; CHECK-LABEL: Determining loop execution counts for: @max_overflow
-; CHECK: Loop %loop: backedge-taken count is (-126 + (126 smax %n))<nsw>
+; CHECK: Loop %loop: backedge-taken count is 0
 ; CHECK: Loop %loop: max backedge-taken count is 0
 entry:
   br label %loop
Index: llvm/lib/Analysis/ScalarEvolution.cpp
===================================================================
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -11947,6 +11947,10 @@
   } else {
     MaxBECount = computeMaxBECountForLT(
         Start, Stride, RHS, getTypeSizeInBits(LHS->getType()), IsSigned);
+    // If we prove the max count is zero, so is the symbolic bound.  This can
+    // happen due to differences in how we reason about bounds impied by UB.
+    if (MaxBECount->isZero())
+      BECount = MaxBECount;
   }
 
   if (isa<SCEVCouldNotCompute>(MaxBECount) &&


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108921.369462.patch
Type: text/x-patch
Size: 1204 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210830/fc16c047/attachment.bin>