[PATCH] D38825: [SCEV] Teach SCEV to find maxBECount when loop endbound is variant

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9693
+        IsSigned ? getSignedRangeMin(Stride) : getUnsignedRangeMin(Stride);
+  else
+    // Using a stride of 1 is safe when computing max backedge taken count for
----------------
mkazantsev wrote:
> If the stride is known negative and `IsSigned` is `true`, can we return `computeMaxBECount(End, -Stride, Start, BitWidth, IsSigned)` instead of conservative estimate?
This code is never used for zero value stride or `isKnownNegative`. Please see line 9787 where we bail out on zero value and negative strides.

The computeMaxBE code is an NFC refactoring of existing maxBECount calculation when exactBECount is not known. This patch just uses the existing code for variant RHS as well, and relies on already existing overflow checks.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9694
+  else
+    // Using a stride of 1 is safe when computing max backedge taken count for
+    // a loop with unknown stride.
----------------
mkazantsev wrote:
> Do we have a guarantee that stride is not zero? I think we should bail early unless we proved it or assert on that.
yes, as mentioned in response above. I'll add an assert.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9808
+  if (!isLoopInvariant(RHS, L)) {
+    const SCEV *MaxBECount = computeMaxBECount(
+        Start, Stride, RHS, getTypeSizeInBits(LHS->getType()), IsSigned);
----------------
mkazantsev wrote:
> Do we have a guarantee that the IV increment has no wrap flag set? Otherwise I don't believe it works for stride other than 1. For example:
> 
>   L = load volatile ...
>   for (int i = 0; i < L; i += 2) {
>     ...
>     L = load volatile ...
>   }
> 
> If `L` is always `SINT_MAX`, this loop is infinite (if IV increment doesn't have `nsw` flag set).
> 
> For stride = 1 I think it should work fine even without `nsw`: we always stop when `i = SINT_MAX` at most, but for other step values we can leap across it.
All the overflow tests are already done above and it handles the case you mention: stride > 1, stride is not positive etc. For clarity, that's the comment added just above this code at line 9806. 

Yes, for stride =1, we are fine without nsw because of the strictly LT condition. I've also added a testcase for that.

I will add a testcase for stride > 1, without nsw flag and confirm what I've said is true.


https://reviews.llvm.org/D38825