[SCEV] forgetValue: support (with-overflow-inst op0, op1) #98015
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
v01dXYZ
changed the title
|
Yes, I verified with the input you provided in your issue. The program doesn't abort. Input; ModuleID = '<stdin>'
source_filename = "<stdin>"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
target triple = "x86_64-unknown-linux-gnu"
define void @ham() {
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
}
; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
declare { i32, i1 } @llvm.smul.with.overflow.i32(i32, i32) #0
attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }~/llvm-project/build/bin/opt <filename.ll> \
-S -debug --print-before-all \
--passes="loop-unroll<no-partial;peeling;no-runtime;no-upperbound;no-profile-peeling;full-unroll-max=0;O3>" |
nikic
reviewed
Jul 8, 2024
nikic
reviewed
Jul 8, 2024
v01dXYZ
force-pushed
the
97586-scev-bug-forgetvalue-with-overflow-inst
branch
from
45e5518 to
db89a5e
Compare
…nst 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.
v01dXYZ
force-pushed
the
97586-scev-bug-forgetvalue-with-overflow-inst
branch
from
db89a5e to
764fc0b
Compare
nikic
approved these changes
Jul 8, 2024
llvm/test/Transforms/LoopUnroll/peel-loop-scev-invalidate-with-overflow-inst.ll
Outdated
Show resolved
Hide resolved
v01dXYZ
force-pushed
the
97586-scev-bug-forgetvalue-with-overflow-inst
branch
from
4c2cff6 to
000a4a6
Compare
added 3 commits
July 8, 2024 22:18
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.
v01dXYZ
force-pushed
the
97586-scev-bug-forgetvalue-with-overflow-inst
branch
from
000a4a6 to
3ef1bf6
Compare
v01dXYZ
changed the title
v01dXYZ
changed the title
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-transforms Author: None (v01dXYZ) ChangesFixes #97586.
Full diff: https://github.com/llvm/llvm-project/pull/98015.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 430e1c6d8f8c6..51cffac808768 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -8412,7 +8412,7 @@ void ScalarEvolution::visitAndClearUsers(
SmallVectorImpl<const SCEV *> &ToForget) {
while (!Worklist.empty()) {
Instruction *I = Worklist.pop_back_val();
- if (!isSCEVable(I->getType()))
+ if (!isSCEVable(I->getType()) && !isa<WithOverflowInst>(I))
continue;
ValueExprMapType::iterator It =
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..b14dda541e065
--- /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 * %add8.lcssa)
+; CHECK: %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
+; CHECK-NEXT: --> {0,+,(-2 * %add8.lcssa)}<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
+}
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
|
|
@llvm/pr-subscribers-llvm-analysis Author: None (v01dXYZ) ChangesFixes #97586.
Full diff: https://github.com/llvm/llvm-project/pull/98015.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 430e1c6d8f8c6..51cffac808768 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -8412,7 +8412,7 @@ void ScalarEvolution::visitAndClearUsers(
SmallVectorImpl<const SCEV *> &ToForget) {
while (!Worklist.empty()) {
Instruction *I = Worklist.pop_back_val();
- if (!isSCEVable(I->getType()))
+ if (!isSCEVable(I->getType()) && !isa<WithOverflowInst>(I))
continue;
ValueExprMapType::iterator It =
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..b14dda541e065
--- /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 * %add8.lcssa)
+; CHECK: %phi4 = phi i32 [ %add, %bb3 ], [ 0, %bb2 ]
+; CHECK-NEXT: --> {0,+,(-2 * %add8.lcssa)}<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
+}
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
|
|
@v01dXYZ Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
The use-def walk in forgetValue() was skipping instructions with non-SCEVable types. However, SCEV may look past with.overflow intrinsics returning aggregates. Fixes llvm#97586.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #97586.
The fix is not satisfactory as there is code duplication and it could get out of sync withMatchBinaryOpin the future if new patterns are added. The useful part of this commit is the test.