[PATCH] D22377: [SCEV] trip count calculation for loops with unknown stride

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:4956
@@ +4955,3 @@
+        // Non-atomic, non-volatile stores are ok.
+        if (auto SI = dyn_cast<StoreInst>(&I)) {
+          return SI->isSimple();
----------------
No braces needed here.  Also, I'd drop the `else`, and just have:

```
if (X)
  return Y;
return Z;
```


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:4956
@@ +4955,3 @@
+        // Non-atomic, non-volatile stores are ok.
+        if (auto SI = dyn_cast<StoreInst>(&I)) {
+          return SI->isSimple();
----------------
sanjoy wrote:
> No braces needed here.  Also, I'd drop the `else`, and just have:
> 
> ```
> if (X)
>   return Y;
> return Z;
> ```
> 
Use `auto *` to denote that `SI` is a pointer.

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8731
@@ -8704,2 +8730,3 @@
   // Avoid negative or zero stride values
-  if (!isKnownPositive(Stride))
+  // We can assume stride to be positive if NoWrap is true and loop does not
+  // have abnormal exits or side effects.
----------------
As we discussed, it isn't that given the conditions you're testing  we can prove that the stride is non-negative, but that the math below is correct even if the stride is negative.

Also, I'd separate out the `!NoWrap || !loopHasNoAbnormalExits(L) || !loopHasNoSideEffects(L)` check into it's own `if` and have a small comment describing why they're necessary / sufficient (the example you had in the llvm-dev thread would be nice to add too).

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8751
@@ +8750,3 @@
+    // do-while loop. Stride cannot be assumed to be positive for such loops so
+    // we bail out.
+    if (!PositiveStride) 
----------------
Here and in the comment in the test case, we're not assuming the stride is positive, but that the trip count math is correct even if stride is negative given that the conditions we checked above are correct; so I think the comment and the variable name needs to change.




https://reviews.llvm.org/D22377