[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul. #91472

fhahn · 2024-05-08T13:47:08Z

SCEVLoopGuardRewriter only replaces operands with equivalent values, so we should be able to transfer the flags from the original expression.

llvmbot · 2024-05-08T13:47:41Z

@llvm/pr-subscribers-llvm-analysis

Author: Florian Hahn (fhahn)

Changes

SCEVLoopGuardRewriter only replaces operands with equivalent values, so we should be able to transfer the flags from the original expression.

Full diff: https://github.com/llvm/llvm-project/pull/91472.diff

3 Files Affected:

(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+24)
(modified) llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll (+4-4)
(modified) llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll (+1-2)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 93f885c5d5ad8..3666a7d4b9b22 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -15034,6 +15034,30 @@ class SCEVLoopGuardRewriter : public SCEVRewriteVisitor<SCEVLoopGuardRewriter> {
       return SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSMinExpr(Expr);
     return I->second;
   }
+
+  const SCEV *visitAddExpr(const SCEVAddExpr *Expr) {
+    SmallVector<const SCEV *, 2> Operands;
+    bool Changed = false;
+    for (const auto *Op : Expr->operands()) {
+      Operands.push_back(SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visit(Op));
+      Changed |= Op != Operands.back();
+    }
+    // We are only replacing operands with equivalent values, so transfer the
+    // flags from the original expression.
+    return !Changed ? Expr : SE.getAddExpr(Operands, Expr->getNoWrapFlags());
+  }
+
+  const SCEV *visitMulExpr(const SCEVMulExpr *Expr) {
+    SmallVector<const SCEV *, 2> Operands;
+    bool Changed = false;
+    for (const auto *Op : Expr->operands()) {
+      Operands.push_back(SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visit(Op));
+      Changed |= Op != Operands.back();
+    }
+    // We are only replacing operands with equivalent values, so transfer the
+    // flags from the original expression.
+    return !Changed ? Expr : SE.getMulExpr(Operands, Expr->getNoWrapFlags());
+  }
 };
 
 const SCEV *ScalarEvolution::applyLoopGuards(const SCEV *Expr, const Loop *L) {
diff --git a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
index da4487ce9cd48..3c3748a6a5f02 100644
--- a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
+++ b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
@@ -77,13 +77,13 @@ define void @rewrite_preserve_add_nsw(i32 %a) {
 ; CHECK-NEXT:    %add = add nsw i32 %a, 4
 ; CHECK-NEXT:    --> (4 + %a)<nsw> U: [-2147483644,-2147483648) S: [-2147483644,-2147483648)
 ; CHECK-NEXT:    %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]
-; CHECK-NEXT:    --> {0,+,1}<nuw><%loop> U: [0,-2147483648) S: [0,-2147483648) Exits: (0 smax (4 + %a)<nsw>) LoopDispositions: { %loop: Computable }
+; CHECK-NEXT:    --> {0,+,1}<nuw><%loop> U: [0,-2147483648) S: [0,-2147483648) Exits: (4 + %a)<nsw> LoopDispositions: { %loop: Computable }
 ; CHECK-NEXT:    %iv.next = add i32 %iv, 1
-; CHECK-NEXT:    --> {1,+,1}<nuw><%loop> U: [1,-2147483647) S: [1,-2147483647) Exits: (1 + (0 smax (4 + %a)<nsw>))<nuw> LoopDispositions: { %loop: Computable }
+; CHECK-NEXT:    --> {1,+,1}<nuw><%loop> U: [1,-2147483647) S: [1,-2147483647) Exits: (5 + %a) LoopDispositions: { %loop: Computable }
 ; CHECK-NEXT:  Determining loop execution counts for: @rewrite_preserve_add_nsw
-; CHECK-NEXT:  Loop %loop: backedge-taken count is (0 smax (4 + %a)<nsw>)
+; CHECK-NEXT:  Loop %loop: backedge-taken count is (4 + %a)<nsw>
 ; CHECK-NEXT:  Loop %loop: constant max backedge-taken count is i32 2147483647
-; CHECK-NEXT:  Loop %loop: symbolic max backedge-taken count is (0 smax (4 + %a)<nsw>)
+; CHECK-NEXT:  Loop %loop: symbolic max backedge-taken count is (4 + %a)<nsw>
 ; CHECK-NEXT:  Loop %loop: Trip multiple is 1
 ;
 entry:
diff --git a/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll b/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
index f86639ea4c50f..945eb82e18595 100644
--- a/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
+++ b/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
@@ -12,8 +12,7 @@ define void @rewrite_preserve_add_nsw(i32 %a) {
 ; CHECK-NEXT:    [[PRE:%.*]] = icmp sgt i32 [[A]], -4
 ; CHECK-NEXT:    br i1 [[PRE]], label [[LOOP_PREHEADER:%.*]], label [[EXIT:%.*]]
 ; CHECK:       loop.preheader:
-; CHECK-NEXT:    [[SMAX:%.*]] = call i32 @llvm.smax.i32(i32 [[ADD]], i32 0)
-; CHECK-NEXT:    [[TMP0:%.*]] = add nuw i32 [[SMAX]], 1
+; CHECK-NEXT:    [[TMP0:%.*]] = add i32 [[A]], 5
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ [[IV_NEXT:%.*]], [[LOOP]] ], [ 0, [[LOOP_PREHEADER]] ]

llvmbot · 2024-05-08T13:47:42Z

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

SCEVLoopGuardRewriter only replaces operands with equivalent values, so we should be able to transfer the flags from the original expression.

Full diff: https://github.com/llvm/llvm-project/pull/91472.diff

3 Files Affected:

(modified) llvm/lib/Analysis/ScalarEvolution.cpp (+24)
(modified) llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll (+4-4)
(modified) llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll (+1-2)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 93f885c5d5ad8..3666a7d4b9b22 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -15034,6 +15034,30 @@ class SCEVLoopGuardRewriter : public SCEVRewriteVisitor<SCEVLoopGuardRewriter> {
       return SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visitSMinExpr(Expr);
     return I->second;
   }
+
+  const SCEV *visitAddExpr(const SCEVAddExpr *Expr) {
+    SmallVector<const SCEV *, 2> Operands;
+    bool Changed = false;
+    for (const auto *Op : Expr->operands()) {
+      Operands.push_back(SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visit(Op));
+      Changed |= Op != Operands.back();
+    }
+    // We are only replacing operands with equivalent values, so transfer the
+    // flags from the original expression.
+    return !Changed ? Expr : SE.getAddExpr(Operands, Expr->getNoWrapFlags());
+  }
+
+  const SCEV *visitMulExpr(const SCEVMulExpr *Expr) {
+    SmallVector<const SCEV *, 2> Operands;
+    bool Changed = false;
+    for (const auto *Op : Expr->operands()) {
+      Operands.push_back(SCEVRewriteVisitor<SCEVLoopGuardRewriter>::visit(Op));
+      Changed |= Op != Operands.back();
+    }
+    // We are only replacing operands with equivalent values, so transfer the
+    // flags from the original expression.
+    return !Changed ? Expr : SE.getMulExpr(Operands, Expr->getNoWrapFlags());
+  }
 };
 
 const SCEV *ScalarEvolution::applyLoopGuards(const SCEV *Expr, const Loop *L) {
diff --git a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
index da4487ce9cd48..3c3748a6a5f02 100644
--- a/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
+++ b/llvm/test/Analysis/ScalarEvolution/backedge-taken-count-guard-info.ll
@@ -77,13 +77,13 @@ define void @rewrite_preserve_add_nsw(i32 %a) {
 ; CHECK-NEXT:    %add = add nsw i32 %a, 4
 ; CHECK-NEXT:    --> (4 + %a)<nsw> U: [-2147483644,-2147483648) S: [-2147483644,-2147483648)
 ; CHECK-NEXT:    %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]
-; CHECK-NEXT:    --> {0,+,1}<nuw><%loop> U: [0,-2147483648) S: [0,-2147483648) Exits: (0 smax (4 + %a)<nsw>) LoopDispositions: { %loop: Computable }
+; CHECK-NEXT:    --> {0,+,1}<nuw><%loop> U: [0,-2147483648) S: [0,-2147483648) Exits: (4 + %a)<nsw> LoopDispositions: { %loop: Computable }
 ; CHECK-NEXT:    %iv.next = add i32 %iv, 1
-; CHECK-NEXT:    --> {1,+,1}<nuw><%loop> U: [1,-2147483647) S: [1,-2147483647) Exits: (1 + (0 smax (4 + %a)<nsw>))<nuw> LoopDispositions: { %loop: Computable }
+; CHECK-NEXT:    --> {1,+,1}<nuw><%loop> U: [1,-2147483647) S: [1,-2147483647) Exits: (5 + %a) LoopDispositions: { %loop: Computable }
 ; CHECK-NEXT:  Determining loop execution counts for: @rewrite_preserve_add_nsw
-; CHECK-NEXT:  Loop %loop: backedge-taken count is (0 smax (4 + %a)<nsw>)
+; CHECK-NEXT:  Loop %loop: backedge-taken count is (4 + %a)<nsw>
 ; CHECK-NEXT:  Loop %loop: constant max backedge-taken count is i32 2147483647
-; CHECK-NEXT:  Loop %loop: symbolic max backedge-taken count is (0 smax (4 + %a)<nsw>)
+; CHECK-NEXT:  Loop %loop: symbolic max backedge-taken count is (4 + %a)<nsw>
 ; CHECK-NEXT:  Loop %loop: Trip multiple is 1
 ;
 entry:
diff --git a/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll b/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
index f86639ea4c50f..945eb82e18595 100644
--- a/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
+++ b/llvm/test/Transforms/IndVarSimplify/trip-count-expansion-loop-guard-preserve-nsw.ll
@@ -12,8 +12,7 @@ define void @rewrite_preserve_add_nsw(i32 %a) {
 ; CHECK-NEXT:    [[PRE:%.*]] = icmp sgt i32 [[A]], -4
 ; CHECK-NEXT:    br i1 [[PRE]], label [[LOOP_PREHEADER:%.*]], label [[EXIT:%.*]]
 ; CHECK:       loop.preheader:
-; CHECK-NEXT:    [[SMAX:%.*]] = call i32 @llvm.smax.i32(i32 [[ADD]], i32 0)
-; CHECK-NEXT:    [[TMP0:%.*]] = add nuw i32 [[SMAX]], 1
+; CHECK-NEXT:    [[TMP0:%.*]] = add i32 [[A]], 5
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ [[IV_NEXT:%.*]], [[LOOP]] ], [ 0, [[LOOP_PREHEADER]] ]

PR Link: llvm/llvm-project#91472

efriedma-quic · 2024-05-08T19:44:11Z

Should this also handle AddRecExpr?

fhahn · 2024-05-08T20:24:47Z

Should this also handle AddRecExpr?

The default rewriter implementation already adds the flags https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Analysis/ScalarEvolutionExpressions.h#L836 (not sure if that is safe in all cases)

nikic · 2024-05-09T00:49:01Z

SCEVLoopGuardRewriter only replaces operands with equivalent values

But aren't the values only equivalent in the scope of the guards, while the generated expressions might potentially have a larger scope?

fhahn · 2024-05-09T12:52:48Z

SCEVLoopGuardRewriter only replaces operands with equivalent values

But aren't the values only equivalent in the scope of the guards, while the generated expressions might potentially have a larger scope?

The patch still uses the flags from the original expression which should be valid for their original scope, so after re-writing with more restricted values they should still be valid for the larger scope (e.g. re-writing a + nuw b => a +nuw c with range(b).contains(range(c))?

nikic · 2024-05-09T13:16:13Z

SCEVLoopGuardRewriter only replaces operands with equivalent values

But aren't the values only equivalent in the scope of the guards, while the generated expressions might potentially have a larger scope?

The patch still uses the flags from the original expression which should be valid for their original scope, so after re-writing with more restricted values they should still be valid for the larger scope (e.g. re-writing a + nuw b => a +nuw c with range(b).contains(range(c))?

Is it true though that the range is strictly smaller though? If we have a +nuw b and you rewrite this to a +nuw umax(b, c) based on a b >= c constraint, can't a +nuw c unsigned wrap outside the constrained scope?

…flags

fhahn · 2024-05-10T18:26:08Z

Is it true though that the range is strictly smaller though? If we have a +nuw b and you rewrite this to a +nuw umax(b, c) based on a b >= c constraint, can't a +nuw c unsigned wrap outside the constrained scope?

I originally thought so, but there are some cases where that's not the case when checking via asserts (not sure if the replacement helps in general in such cases thought).

Updated this PR to check if the replacement ranges are contained in the replaced ranges and only preserve NSW/NUW if they are

fhahn · 2024-05-16T15:13:41Z

ping :)

fhahn · 2024-05-24T09:54:22Z

ping :)

fhahn · 2024-05-30T13:58:24Z

ping

nikic

LGTM

[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul.

90a2860

SCEVLoopGuardRewriter only replaces operands with equivalent values, so we should be able to transfer the flags from the original expression.

fhahn requested review from preames and efriedma-quic May 8, 2024 13:47

fhahn requested a review from nikic as a code owner May 8, 2024 13:47

llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels May 8, 2024

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request May 8, 2024

pre-commit: test PR91472

8c42ba8

PR Link: llvm/llvm-project#91472

dtcxzyw mentioned this pull request May 8, 2024

pre-commit: test PR91472 dtcxzyw/llvm-opt-benchmark#593

Closed

fhahn added 2 commits May 10, 2024 15:58

Merge remote-tracking branch 'origin/main' into scev-guards-preserve-…

995878d

…flags

!fiuxp preserve flags only if ranges are contained.

339f8b9

nikic approved these changes Jun 3, 2024

View reviewed changes

fhahn merged commit 4812e9a into llvm:main Jun 3, 2024
4 checks passed

fhahn deleted the scev-guards-preserve-flags branch June 3, 2024 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul. #91472

[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul. #91472

Uh oh!

fhahn commented May 8, 2024

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

efriedma-quic commented May 8, 2024

Uh oh!

fhahn commented May 8, 2024

Uh oh!

nikic commented May 9, 2024

Uh oh!

fhahn commented May 9, 2024

Uh oh!

nikic commented May 9, 2024

Uh oh!

fhahn commented May 10, 2024

Uh oh!

fhahn commented May 16, 2024

Uh oh!

fhahn commented May 24, 2024

Uh oh!

fhahn commented May 30, 2024

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul. #91472

[SCEV] Preserve flags in SCEVLoopGuardRewriter for add and mul. #91472

Uh oh!

Conversation

fhahn commented May 8, 2024

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

efriedma-quic commented May 8, 2024

Uh oh!

fhahn commented May 8, 2024

Uh oh!

nikic commented May 9, 2024

Uh oh!

fhahn commented May 9, 2024

Uh oh!

nikic commented May 9, 2024

Uh oh!

fhahn commented May 10, 2024

Uh oh!

fhahn commented May 16, 2024

Uh oh!

fhahn commented May 24, 2024

Uh oh!

fhahn commented May 30, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!