[llvm] (Draft) [SCEV] forgetValue: support (extractvalue 0, (with-overflow-inst op0, op1)) (PR #98015)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 8 13:18:26 PDT 2024
https://github.com/v01dXYZ updated https://github.com/llvm/llvm-project/pull/98015
>From d82945c920c492cce16834fa18f6dafcadca8536 Mon Sep 17 00:00:00 2001
From: v01dxyz <v01dxyz at v01d.xyz>
Date: Mon, 8 Jul 2024 12:19:52 +0200
Subject: [PATCH 1/4] (Draft) [SCEV] forgetValue: support (extractvalue 0,
(with-overflow-inst op0, op1))
Without that, forgetValue stops at (with-overflow-inst op0, op1) while
thanks to MatchBinaryOp, SCEV creation considers %extractvalue as if
it is (op op0, op1).
Because of that, it creates an unclearable SCEV value that could
possibly turn out of sync after a transform is applied.
The commit is in Draft as the fix is not satisfactory (code
duplication, do we push EVO to Visited ?). Meant for discussion and
for adding a test.
---
llvm/lib/Analysis/ScalarEvolution.cpp | 28 ++++++++++++++-
.../Analysis/ScalarEvolutionTest.cpp | 36 +++++++++++++++++++
2 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 430e1c6d8f8c6..54f45f0da9d97 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -8412,8 +8412,34 @@ void ScalarEvolution::visitAndClearUsers(
SmallVectorImpl<const SCEV *> &ToForget) {
while (!Worklist.empty()) {
Instruction *I = Worklist.pop_back_val();
- if (!isSCEVable(I->getType()))
+ if (!isSCEVable(I->getType())) {
+ // detect if a user is matching the pattern
+ // extractvalue 0, (with-overflow-inst op1, op2))
+ auto *WO = dyn_cast<WithOverflowInst>(I);
+ if (!WO)
+ continue;
+
+ for (auto *WOUser : WO->users()) {
+ auto *EVO = dyn_cast<ExtractValueInst>(WOUser);
+
+ if (!EVO)
+ continue;
+
+ if (EVO->getNumIndices() != 1 || EVO->getIndices()[0] != 0)
+ continue;
+
+ ValueExprMapType::iterator It =
+ ValueExprMap.find_as(static_cast<Value *>(EVO));
+ if (It != ValueExprMap.end()) {
+ eraseValueFromMap(It->first);
+ ToForget.push_back(It->second);
+ }
+
+ PushDefUseChildren(EVO, Worklist, Visited);
+ }
+
continue;
+ }
ValueExprMapType::iterator It =
ValueExprMap.find_as(static_cast<Value *>(I));
diff --git a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp
index a7b3c5c404ab7..a6a5ffda3cb70 100644
--- a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp
+++ b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp
@@ -1589,4 +1589,40 @@ TEST_F(ScalarEvolutionsTest, ApplyLoopGuards) {
});
}
+TEST_F(ScalarEvolutionsTest, ForgetValueWithOverflowInst) {
+ LLVMContext C;
+ SMDiagnostic Err;
+ std::unique_ptr<Module> M = parseAssemblyString(
+ "declare { i32, i1 } @llvm.smul.with.overflow.i32(i32, i32) "
+ "define void @foo(i32 %i) { "
+ "entry: "
+ " br label %loop.body "
+ "loop.body: "
+ " %iv = phi i32 [ %iv.next, %loop.body ], [ 0, %entry ] "
+ " %iv.next = add nsw i32 %iv, 1 "
+ " %call = call {i32, i1} @llvm.smul.with.overflow.i32(i32 %iv, i32 -2) "
+ " %extractvalue = extractvalue {i32, i1} %call, 0 "
+ " %cmp = icmp eq i32 %iv.next, 16 "
+ " br i1 %cmp, label %exit, label %loop.body "
+ "exit: "
+ " ret void "
+ "} ",
+ Err, C);
+
+ ASSERT_TRUE(M && "Could not parse module?");
+ ASSERT_TRUE(!verifyModule(*M) && "Must have been well formed!");
+
+ runWithSE(*M, "foo", [](Function &F, LoopInfo &LI, ScalarEvolution &SE) {
+ auto *ExtractValue = getInstructionByName(F, "extractvalue");
+ auto *IV = getInstructionByName(F, "iv");
+
+ auto *ExtractValueScev = SE.getSCEV(ExtractValue);
+ EXPECT_NE(ExtractValueScev, nullptr);
+
+ SE.forgetValue(IV);
+ auto *ExtractValueScevForgotten = SE.getExistingSCEV(ExtractValue);
+ EXPECT_EQ(ExtractValueScevForgotten, nullptr);
+ });
+}
+
} // end namespace llvm
>From f08d687edc29bb0f4a6ce6d26a6432304038bbbb Mon Sep 17 00:00:00 2001
From: v01dxyz <v01dxyz at v01d.xyz>
Date: Mon, 8 Jul 2024 19:30:35 +0200
Subject: [PATCH 2/4] [LoopUnroll] pre-commit test for LoopPeel SCEV
invalidation
Warning this test passes only with asserts disabled.
Otherwise the following assertion failure occurs:
Assertion `isAvailableAtLoopEntry(Operands[i], L) && "SCEVAddRecExpr operand is not available at loop entry!"' failed.
---
...loop-scev-invalidate-with-overflow-inst.ll | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
diff --git a/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll b/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
new file mode 100644
index 0000000000000..570f5b27d83f3
--- /dev/null
+++ b/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
@@ -0,0 +1,71 @@
+; RUN: opt < %s -S -passes='print<scalar-evolution>,loop-unroll<peeling;full-unroll-max=0>,print<scalar-evolution>' 2>&1 | FileCheck %s
+;
+; This test ensures that (extractvalue 0 (with-overflow-inst op0, op1))
+; is invalidated by LoopPeel when the operands of with-overflow-inst
+; are changed.
+;
+; In the following case, LoopPeel modifies the CFG into another one
+; with %bb7 not dominating %bb2 and %bb3 although %extractvalue is
+; still the step for the %bb3 loop. %call has been modified and uses
+; different operands but the SCEV value for %extractvalue has not been
+; invalidated and still refers to %load in its SCEV operands
+; (SCEV(%extractvalue) := -2 + -2 * %load).
+;
+; When LoopUnroll tries to compute the SCEV for the %bb3 Phi, the
+; stale data for %extractvalue is used whereas %load is not available
+; in %bb3 which is wrong.
+;
+; for more details and nice pictures: https://github.com/llvm/llvm-project/issues/97586
+;
+; Although the reason for the bug was in forgetValue, it is still relevant to
+; test if LoopPeel invalidates %extractvalue after changing %call.
+;
+; forgetValue only walks the users, so calling it on the IV Phis does not
+; invalidate %extractvalue (thus forgetLoop does not invalidate it too).
+; It has to be done by LoopPeel itself.
+
+
+define void @loop_peeling_smul_with_overflow() {
+; before loop-unroll
+; CHECK: Classifying expressions for: @loop_peeling_smul_with_overflow
+; CHECK: %extractvalue = extractvalue { i32, i1 } %call, 0
+; CHECK-NEXT: --> (-2 + (-2 * %load))
+; CHECK: %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
+; CHECK-NEXT: --> {0,+,(-2 + (-2 * %load))}<nuw><nsw><%bb3>
+; after loop-unroll
+; CHECK: Classifying expressions for: @loop_peeling_smul_with_overflow
+; CHECK: %extractvalue = extractvalue { i32, i1 } %call, 0
+; CHECK-NEXT: --> (-2 + (-2 * %load))
+; CHECK: %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
+; CHECK-NEXT: --> {0,+,(-2 + (-2 * %load))}<nuw><nsw><%bb3>
+;
+bb:
+ br label %bb1
+
+bb1: ; preds = %bb3, %bb
+ %phi = phi i32 [ 0, %bb ], [ %phi4, %bb3 ]
+ br label %bb5
+
+bb2: ; preds = %bb7
+ %call = call { i32, i1 } @llvm.smul.with.overflow.i32(i32 %add8, i32 -2)
+ %extractvalue = extractvalue { i32, i1 } %call, 0
+ br label %bb3
+
+bb3: ; preds = %bb3, %bb2
+ %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
+ %add = add i32 %extractvalue, %phi4
+ br i1 false, label %bb3, label %bb1
+
+bb5: ; preds = %bb7, %bb1
+ %phi6 = phi i32 [ 1, %bb1 ], [ 0, %bb7 ]
+ %icmp = icmp eq i32 %phi, 0
+ br i1 %icmp, label %bb9, label %bb7
+
+bb7: ; preds = %bb5
+ %load = load i32, ptr addrspace(1) null, align 4
+ %add8 = add i32 %load, 1
+ br i1 false, label %bb2, label %bb5
+
+bb9: ; preds = %bb5
+ ret void
+}
>From 612b1cdee721d8edd11dc6dddea57951c45bad82 Mon Sep 17 00:00:00 2001
From: v01dxyz <v01dxyz at v01d.xyz>
Date: Mon, 8 Jul 2024 19:49:58 +0200
Subject: [PATCH 3/4] [LoopUnroll] test for LoopPeel SCEV invalidation
---
.../peel-loop-scev-invalidate-with-overflow-inst.ll | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll b/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
index 570f5b27d83f3..b14dda541e065 100644
--- a/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
+++ b/llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
@@ -35,9 +35,9 @@ define void @loop_peeling_smul_with_overflow() {
; after loop-unroll
; CHECK: Classifying expressions for: @loop_peeling_smul_with_overflow
; CHECK: %extractvalue = extractvalue { i32, i1 } %call, 0
-; CHECK-NEXT: --> (-2 + (-2 * %load))
+; CHECK-NEXT: --> (-2 * %add8.lcssa)
; CHECK: %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
-; CHECK-NEXT: --> {0,+,(-2 + (-2 * %load))}<nuw><nsw><%bb3>
+; CHECK-NEXT: --> {0,+,(-2 * %add8.lcssa)}<nuw><nsw><%bb3>
;
bb:
br label %bb1
>From 3ef1bf698ac7d1584f26277853ebab5b0c15db63 Mon Sep 17 00:00:00 2001
From: v01dxyz <v01dxyz at v01d.xyz>
Date: Mon, 8 Jul 2024 20:33:03 +0200
Subject: [PATCH 4/4] [SCEV] visitAndClearUsers support WithOverflowInst more
simply
---
llvm/lib/Analysis/ScalarEvolution.cpp | 28 +--------------------------
1 file changed, 1 insertion(+), 27 deletions(-)
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 54f45f0da9d97..51cffac808768 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -8412,34 +8412,8 @@ void ScalarEvolution::visitAndClearUsers(
SmallVectorImpl<const SCEV *> &ToForget) {
while (!Worklist.empty()) {
Instruction *I = Worklist.pop_back_val();
- if (!isSCEVable(I->getType())) {
- // detect if a user is matching the pattern
- // extractvalue 0, (with-overflow-inst op1, op2))
- auto *WO = dyn_cast<WithOverflowInst>(I);
- if (!WO)
- continue;
-
- for (auto *WOUser : WO->users()) {
- auto *EVO = dyn_cast<ExtractValueInst>(WOUser);
-
- if (!EVO)
- continue;
-
- if (EVO->getNumIndices() != 1 || EVO->getIndices()[0] != 0)
- continue;
-
- ValueExprMapType::iterator It =
- ValueExprMap.find_as(static_cast<Value *>(EVO));
- if (It != ValueExprMap.end()) {
- eraseValueFromMap(It->first);
- ToForget.push_back(It->second);
- }
-
- PushDefUseChildren(EVO, Worklist, Visited);
- }
-
+ if (!isSCEVable(I->getType()) && !isa<WithOverflowInst>(I))
continue;
- }
ValueExprMapType::iterator It =
ValueExprMap.find_as(static_cast<Value *>(I));
More information about the llvm-commits
mailing list