[PATCH] D130728: [SCEV] Iteratively compute ranges for deeply nested expressions.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6437
+    for (const SCEV *P :
+         reverse(make_range(WorkList.begin() + 1, WorkList.end()))) {
+      getRangeRef(P, SignHint);
----------------
hiraditya wrote:
> Can the traversal order be preserved if we populated the Worklist with DFS traversal for operands starting with `S`?
I had a look at some of the differences and the main issue is that we now may evaluate ranges earlier than previously.

One example is that for AddRecs, `getRangeRef` may not evaluate the range of the start value, e.g. because it may not be necessary if the addrec overflows. With the iterative approach, we will evaluate the range of the start value before evaluating the addrec.  I don't think there anything we could do about it, except adding the expression-dependent logic of when to not evaluate an operand.

I think this is undesirable and should also not be necessary in practice. The differences caused by this are very minor (in some cases it even leads to small improvements) and won't materialize except in very deep expressions. I am also open to increasing the threshold when the iterative logic triggers. SCEV should just not overflow the stack for valid IR inputs.

Here's where the ranges would be a bit tighter if we unconditionally use the iterative approach:

```
diff --git a/llvm/test/Analysis/ScalarEvolution/pr49856.ll b/llvm/test/Analysis/ScalarEvolution/pr49856.ll
index 751677f1f9f8..661fd5482ad5 100644
--- a/llvm/test/Analysis/ScalarEvolution/pr49856.ll
+++ b/llvm/test/Analysis/ScalarEvolution/pr49856.ll
@@ -5,7 +5,7 @@ define void @test() {
 ; CHECK-LABEL: 'test'
 ; CHECK-NEXT:  Classifying expressions for: @test
 ; CHECK-NEXT:    %tmp = phi i32 [ 2, %bb ], [ %tmp2, %bb3 ]
-; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [0,-2147483648)
+; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [1,-2147483648)
 ; CHECK-NEXT:    %tmp2 = add nuw nsw i32 %tmp, 1
 ; CHECK-NEXT:    --> (1 + %tmp)<nuw> U: [1,-2147483647) S: [1,-2147483647)
 ; CHECK-NEXT:  Determining loop execution counts for: @test
diff --git a/llvm/test/Analysis/ScalarEvolution/shift-recurrences.ll b/llvm/test/Analysis/ScalarEvolution/shift-recurrences.ll
index bc0f62e827ea..e5199e027ab3 100644
--- a/llvm/test/Analysis/ScalarEvolution/shift-recurrences.ll
+++ b/llvm/test/Analysis/ScalarEvolution/shift-recurrences.ll
@@ -446,7 +446,7 @@ define void @nonloop_recurrence() {
 ; CHECK-LABEL: 'nonloop_recurrence'
 ; CHECK-NEXT:  Classifying expressions for: @nonloop_recurrence
 ; CHECK-NEXT:    %tmp = phi i32 [ 2, %bb ], [ %tmp2, %bb3 ]
-; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [0,-2147483648)
+; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [1,-2147483648)
 ; CHECK-NEXT:    %tmp2 = add nuw nsw i32 %tmp, 1
 ; CHECK-NEXT:    --> (1 + %tmp)<nuw> U: [1,-2147483647) S: [1,-2147483647)
 ; CHECK-NEXT:  Determining loop execution counts for: @nonloop_recurrence
@@ -470,7 +470,7 @@ define void @nonloop_recurrence_2() {
 ; CHECK-LABEL: 'nonloop_recurrence_2'
 ; CHECK-NEXT:  Classifying expressions for: @nonloop_recurrence_2
 ; CHECK-NEXT:    %tmp = phi i32 [ 2, %loop ], [ %tmp2, %bb3 ]
-; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [0,-2147483648) Exits: <<Unknown>> LoopDispositions: { %loop: Variant }
+; CHECK-NEXT:    --> %tmp U: [1,-2147483648) S: [1,-2147483648) Exits: <<Unknown>> LoopDispositions: { %loop: Variant }
 ; CHECK-NEXT:    %tmp2 = add nuw nsw i32 %tmp, 1
 ; CHECK-NEXT:    --> (1 + %tmp)<nuw> U: [1,-2147483647) S: [1,-2147483647) Exits: <<Unknown>> LoopDispositions: { %loop: Variant }
 ; CHECK-NEXT:  Determining loop execution counts for: @nonloop_recurrence_2
```


The only regression AFAICT would be in `llvm/test/Analysis/ScalarEvolution/addrec-computed-during-addrec-calculation.ll` where we fail to hoist the `sext` out of `(sext i32 {%iv,+,1}<nsw><%loop2> to i64)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130728