[JumpThreading] Fix lifetime markers when alloca requires SSA renaming#188147
Conversation
JumpThreading can create PHI nodes for alloca values when threading across blocks. This violates the requirement introduced in llvm#149310 that lifetime.start/end intrinsics must operate directly on allocas, not on PHI nodes. Fix this in two ways: - For allocas with a constant size, move both the original alloca and its clone in the threaded block to the entry block before SSA reconstruction, so they dominate all uses and lifetime markers remain valid after threading. - For any remaining allocas where a lifetime marker is no longer dominated by the alloca after jump threading, remove all lifetime markers. Fixes llvm#167733
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. 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 permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. 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. |
|
@llvm/pr-subscribers-llvm-transforms Author: Guilherme Lopes (guilhermeglopess) ChangesJumpThreading can create PHI nodes for alloca values when threading across blocks. This violates the requirement introduced in #149310 that lifetime.start/end intrinsics must operate directly on allocas, not on PHI nodes. Fix this in two ways:
Fixes #167733 Full diff: https://github.com/llvm/llvm-project/pull/188147.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 7f8e10eb201a6..3367d78c3a657 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -1975,6 +1975,32 @@ void JumpThreadingPass::updateSSA(BasicBlock *BB, BasicBlock *NewBB,
SmallVector<Use *, 16> UsesToRename;
SmallVector<DbgVariableRecord *, 4> DbgVariableRecords;
+ // Move static allocas with lifetime markers to the entry block so they
+ // dominate all their uses after jump threading, preserving lifetime info.
+ // For dynamic allocas, lifetime markers will be dropped below.
+ BasicBlock &EntryBB = BB->getParent()->getEntryBlock();
+ SmallVector<AllocaInst *> AllocasToHoist;
+ for (Instruction &I : *BB) {
+ auto *AI = dyn_cast<AllocaInst>(&I);
+ // Use isa<ConstantInt> instead of isStaticAlloca() because the alloca
+ // is not in the entry block yet, so isStaticAlloca() would return false
+ // even for allocas with a constant size.
+ if (AI && isa<ConstantInt>(AI->getArraySize()) &&
+ any_of(AI->users(), [](User *U) {
+ return cast<Instruction>(U)->isLifetimeStartOrEnd();
+ }))
+ AllocasToHoist.push_back(AI);
+ }
+
+ if (!AllocasToHoist.empty()) {
+ auto HoistPoint = EntryBB.getFirstNonPHIOrDbgOrAlloca()->getIterator();
+ for (AllocaInst *AI : AllocasToHoist) {
+ AI->moveBefore(HoistPoint);
+ if (auto *NewAI = dyn_cast_or_null<AllocaInst>(ValueMapping[AI]))
+ NewAI->moveBefore(HoistPoint);
+ }
+ }
+
for (Instruction &I : *BB) {
// Scan all uses of this instruction to see if it is used outside of its
// block, and if so, record them in UsesToRename.
@@ -2000,6 +2026,24 @@ void JumpThreadingPass::updateSSA(BasicBlock *BB, BasicBlock *NewBB,
continue;
LLVM_DEBUG(dbgs() << "JT: Renaming non-local uses of: " << I << "\n");
+ if (isa<AllocaInst>(&I)) {
+ // Static allocas with lifetime markers were moved to the entry block
+ // above. For any remaining allocas where a lifetime marker is no longer
+ // dominated by the alloca, remove all lifetime markers.
+ DominatorTree &DT = DTU->getDomTree();
+ bool HasNonDominatedLifetimeMarker = any_of(I.users(), [&](User *U) {
+ auto *UserI = cast<Instruction>(U);
+ return UserI->isLifetimeStartOrEnd() && !DT.dominates(&I, UserI);
+ });
+ if (HasNonDominatedLifetimeMarker) {
+ for (User *U : make_early_inc_range(I.users())) {
+ auto *UserI = cast<Instruction>(U);
+ if (UserI->isLifetimeStartOrEnd())
+ UserI->eraseFromParent();
+ }
+ }
+ }
+
// We found a use of I outside of BB. Rename all uses of I that are outside
// its block to be uses of the appropriate PHI node etc. See ValuesInBlocks
// with the two values we know.
diff --git a/llvm/test/Transforms/JumpThreading/lifetime-alloca.ll b/llvm/test/Transforms/JumpThreading/lifetime-alloca.ll
new file mode 100644
index 0000000000000..9a9ce026942b0
--- /dev/null
+++ b/llvm/test/Transforms/JumpThreading/lifetime-alloca.ll
@@ -0,0 +1,40 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=jump-threading -S %s | FileCheck %s
+
+; Test that JumpThreading does not create a PHI node for an alloca that is
+; used by a lifetime intrinsic, which would violate the requirement that
+; lifetime markers only operate on allocas.
+
+define void @widget(i1 %arg) {
+; CHECK-LABEL: @widget(
+; CHECK-NOT: phi ptr
+; CHECK: bb:
+; CHECK-NEXT: %alloca = alloca [4 x [4 x i32]], align 8
+; CHECK-NEXT: %alloca1 = alloca [4 x [4 x i32]], align 8
+; CHECK: call void @llvm.lifetime.start.p0(ptr %alloca)
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(ptr %alloca)
+bb:
+ br i1 %arg, label %bb1, label %bb2
+
+bb1:
+ call void @dummy()
+ br label %bb2
+
+bb2:
+ %alloca = alloca [4 x [4 x i32]], align 8
+ br i1 %arg, label %bb3, label %bb4
+
+bb3:
+ br label %bb4
+
+bb4:
+ call void @llvm.lifetime.start.p0(ptr %alloca)
+ call void @llvm.lifetime.end.p0(ptr %alloca)
+ ret void
+}
+
+declare void @llvm.lifetime.start.p0(ptr captures(none)) #0
+declare void @llvm.lifetime.end.p0(ptr captures(none)) #0
+declare void @dummy()
+
+attributes #0 = { nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
|
| auto *AI = dyn_cast<AllocaInst>(&I); | ||
| // Use isa<ConstantInt> instead of isStaticAlloca() because the alloca | ||
| // is not in the entry block yet, so isStaticAlloca() would return false | ||
| // even for allocas with a constant size. |
There was a problem hiding this comment.
The reason isStaticAlloca checks whether the alloca is in the entry block is that allocas in other blocks can be in a loop, like void *p[10]; for (int i = 0; i < 10; ++i) { p[i] = __builtin_alloca(8); }.
Some allocas can be safely hoisted, but in general hoisting is a miscompile.
There was a problem hiding this comment.
You're right, thanks. I'll simplify the fix to just remove lifetime markers when they become non-dominated after jump threading.
Hoisting allocas to the entry block is unsafe if the alloca is inside a loop. Instead, simply remove all lifetime markers for allocas that would become non-dominated after jump threading.
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
Generally looks reasonable
| if (HasNonDominatedLifetimeMarker) { | ||
| // Remove all uses of lifetime markers from UsesToRename before | ||
| // erasing them. This prevents SSAUpdater from attempting to | ||
| // rewrite uses of instructions that have been deleted. |
There was a problem hiding this comment.
Might make more sense to move this code before UsersToRename is collected in the first place?
There was a problem hiding this comment.
You're right, thanks. Will move it to the initial uses loop to prevent the markers from entering UsesToRename.
Move the check for non-dominated lifetime markers to the first loop to avoid adding them to UsesToRename entirely. Includes a new test case to ensure dominated lifetime markers are preserved during jump threading.
| // alloca after jump threading, we will remove all lifetime markers. | ||
| // This avoids inserting PHI nodes for lifetime markers, which is | ||
| // invalid. | ||
| DominatorTree &DT = DTU->getDomTree(); |
There was a problem hiding this comment.
Looking at this again, I think this call to getDomTree() is problematic, because it will force flushing of domtree updates. We generally don't allow use of the dominator tree inside JumpThreading, because it's expensive to keep it up to date under heavy CFG transforms.
An alternative would be to collect the lifetime intrinsics here, do the transform as usual, and then check if any lifetime intrinsics have a phi argument now. If yes, drop them. It's a bit iffy in that it temporarily produces invalid IR, but given the narrow scope I think it's fine...
There was a problem hiding this comment.
Dropped the DominatorTree check and allowed SSAUpdater to process the markers. Afterwards, if any marker has a PHI node as an argument, we drop the markers for that alloca.
|
Hi @nikic, if you can find the time, I would like your opinion on this |
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
Approach looks fine to me. The PR description needs an update.
guilhermeglopess
force-pushed
the
fix-jump-threading-167733
branch
from
59a0a3f to
4367236
Compare
move UsesToRename outside if block, simplify getOperand, to use default signature, drop strip cast
guilhermeglopess
force-pushed
the
fix-jump-threading-167733
branch
from
4367236 to
5c0778a
Compare
|
Hi @nikic, when you find the time, please let me know if any further changes are needed |
|
Hi @nikic, just following up on this again to see if any changes are required. |
|
@guilhermeglopess 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 by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. 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. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
llvm#188147) JumpThreading can create PHI nodes for alloca values when threading across blocks. This violates the requirement introduced in llvm#149310 that lifetime.start/end intrinsics must operate directly on allocas. After SSA reconstruction, check if any lifetime marker for an alloca now points to a PHI node. If so, drop all lifetime markers for that alloca. Fixes llvm#167733
4 participants
JumpThreading can create PHI nodes for alloca values when threading across blocks. This violates the requirement introduced in #149310 that lifetime.start/end intrinsics must operate directly on allocas.
After SSA reconstruction, check if any lifetime marker for an alloca now points to a PHI node. If so, drop all lifetime markers for that alloca.
Fixes #167733