-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[GVNSink] Do not sink lifetimes of different allocas #149818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This was always undesirable, and after llvm#149310 it is illegal and will result in a verifier error. Fix this by moving SimplifyCFG's check for this into canReplaceOperandWithVariable(), so it's shared with GVNSink.
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesThis was always undesirable, and after #149310 it is illegal and will result in a verifier error. Fix this by moving SimplifyCFG's check for this into canReplaceOperandWithVariable(), so it's shared with GVNSink. Full diff: https://github.com/llvm/llvm-project/pull/149818.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp
index 7f0c23bd24efb..6baa08d43a18b 100644
--- a/llvm/lib/Transforms/Utils/Local.cpp
+++ b/llvm/lib/Transforms/Utils/Local.cpp
@@ -3876,6 +3876,10 @@ bool llvm::canReplaceOperandWithVariable(const Instruction *I, unsigned OpIdx) {
if (Op->isSwiftError())
return false;
+ // Cannot replace alloca argument with phi/select.
+ if (I->isLifetimeStartOrEnd())
+ return false;
+
// Early exit.
if (!isa<Constant, InlineAsm>(Op))
return true;
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 75c96503d556d..94b0ab892f2dd 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2227,16 +2227,6 @@ static bool canSinkInstructions(
return I->getOperand(OI) == I0->getOperand(OI);
};
if (!all_of(Insts, SameAsI0)) {
- // SROA can't speculate lifetime markers of selects/phis, and the
- // backend may handle such lifetimes incorrectly as well (#104776).
- // Don't sink lifetimes if it would introduce a phi on the pointer
- // argument.
- if (isa<LifetimeIntrinsic>(I0) && OI == 1 &&
- any_of(Insts, [](const Instruction *I) {
- return isa<AllocaInst>(I->getOperand(1)->stripPointerCasts());
- }))
- return false;
-
if ((isa<Constant>(Op) && !replacingOperandWithVariableIsCheap(I0, OI)) ||
!canReplaceOperandWithVariable(I0, OI))
// We can't create a PHI from this GEP.
diff --git a/llvm/test/Transforms/GVNSink/lifetime.ll b/llvm/test/Transforms/GVNSink/lifetime.ll
new file mode 100644
index 0000000000000..1a8a69bb0986e
--- /dev/null
+++ b/llvm/test/Transforms/GVNSink/lifetime.ll
@@ -0,0 +1,77 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=gvn-sink < %s | FileCheck %s
+
+; Make sure we do not sink lifetime markers if this would introduce a
+; lifetime with non-alloca operand.
+
+define void @test_cant_sink(i1 %c) {
+; CHECK-LABEL: define void @test_cant_sink(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: [[A:%.*]] = alloca i8, align 1
+; CHECK-NEXT: [[B:%.*]] = alloca i8, align 1
+; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 1, ptr [[A]])
+; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 1, ptr [[B]])
+; CHECK-NEXT: br i1 [[C]], label %[[IF:.*]], label %[[ELSE:.*]]
+; CHECK: [[IF]]:
+; CHECK-NEXT: store i64 1, ptr [[A]], align 4
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 1, ptr [[A]])
+; CHECK-NEXT: br label %[[JOIN:.*]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: store i64 1, ptr [[B]], align 4
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 1, ptr [[B]])
+; CHECK-NEXT: br label %[[JOIN]]
+; CHECK: [[JOIN]]:
+; CHECK-NEXT: ret void
+;
+ %a = alloca i8
+ %b = alloca i8
+ call void @llvm.lifetime.start(i64 1, ptr %a)
+ call void @llvm.lifetime.start(i64 1, ptr %b)
+ br i1 %c, label %if, label %else
+
+if:
+ store i64 1, ptr %a
+ call void @llvm.lifetime.end(i64 1, ptr %a)
+ br label %join
+
+else:
+ store i64 1, ptr %b
+ call void @llvm.lifetime.end(i64 1, ptr %b)
+ br label %join
+
+join:
+ ret void
+}
+
+define void @test_can_sink(i1 %c) {
+; CHECK-LABEL: define void @test_can_sink(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: [[A:%.*]] = alloca i8, align 1
+; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 1, ptr [[A]])
+; CHECK-NEXT: br i1 [[C]], label %[[IF:.*]], label %[[ELSE:.*]]
+; CHECK: [[IF]]:
+; CHECK-NEXT: br label %[[JOIN:.*]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: br label %[[JOIN]]
+; CHECK: [[JOIN]]:
+; CHECK-NEXT: store i64 1, ptr [[A]], align 4
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 1, ptr [[A]])
+; CHECK-NEXT: ret void
+;
+ %a = alloca i8
+ call void @llvm.lifetime.start(i64 1, ptr %a)
+ br i1 %c, label %if, label %else
+
+if:
+ store i64 1, ptr %a
+ call void @llvm.lifetime.end(i64 1, ptr %a)
+ br label %join
+
+else:
+ store i64 1, ptr %a
+ call void @llvm.lifetime.end(i64 1, ptr %a)
+ br label %join
+
+join:
+ ret void
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This was always undesirable, and after #149310 it is illegal and will result in a verifier error.
Fix this by moving SimplifyCFG's check for this into canReplaceOperandWithVariable(), so it's shared with GVNSink.