release/20.x: [CoroSplit] Always erase lifetime intrinsics for spilled allocas (#142551) #147448
Conversation
…m#142551) If the control flow between `lifetime.start` and `lifetime.end` is too complex, it is acceptable to give up the optimization opportunity and collect the alloca to the frame. However, storing to the frame will lengthen the lifetime of the alloca, and the sanitizer will complain. I propose we always erase lifetime intrinsics of spilled allocas. Fix llvm#124612 --------- Co-authored-by: Chuanqi Xu <yedeng.yd@linux.alibaba.com> (cherry picked from commit 038dc2c)
|
@ChuanqiXu9 What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-llvm-transforms Author: None (llvmbot) ChangesBackport 038dc2c Requested by: @ChuanqiXu9 Full diff: https://github.com/llvm/llvm-project/pull/147448.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 68edabb083be3..35832b594e9a3 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1213,11 +1213,17 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
for (const auto &A : FrameData.Allocas) {
AllocaInst *Alloca = A.Alloca;
UsersToUpdate.clear();
- for (User *U : Alloca->users()) {
+ for (User *U : make_early_inc_range(Alloca->users())) {
auto *I = cast<Instruction>(U);
- if (DT.dominates(Shape.CoroBegin, I))
+ // It is meaningless to retain the lifetime intrinsics refer for the
+ // member of coroutine frames and the meaningless lifetime intrinsics
+ // are possible to block further optimizations.
+ if (I->isLifetimeStartOrEnd())
+ I->eraseFromParent();
+ else if (DT.dominates(Shape.CoroBegin, I))
UsersToUpdate.push_back(I);
}
+
if (UsersToUpdate.empty())
continue;
auto *G = GetFramePointer(Alloca);
@@ -1231,17 +1237,8 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
for (auto *DVR : DbgVariableRecords)
DVR->replaceVariableLocationOp(Alloca, G);
- for (Instruction *I : UsersToUpdate) {
- // It is meaningless to retain the lifetime intrinsics refer for the
- // member of coroutine frames and the meaningless lifetime intrinsics
- // are possible to block further optimizations.
- if (I->isLifetimeStartOrEnd()) {
- I->eraseFromParent();
- continue;
- }
-
+ for (Instruction *I : UsersToUpdate)
I->replaceUsesOfWith(Alloca, G);
- }
}
Builder.SetInsertPoint(&*Shape.getInsertPtAfterFramePtr());
for (const auto &A : FrameData.Allocas) {
|
|
Requested from #124612 (comment) |
| // It is meaningless to retain the lifetime intrinsics refer for the | ||
| // member of coroutine frames and the meaningless lifetime intrinsics | ||
| // are possible to block further optimizations. |
There was a problem hiding this comment.
I have trouble parsing this comment. It says it's "meaningless to retain the lifetime intrinsics" and that they may block further optimization, but isn't the point of #124612 that spilling allocas to the coro frame changes the lifetime of the allocas, so that the old lifetime intrinsics are incorrect, and should therefore be removed?
There was a problem hiding this comment.
I feel the point is, the lifetime intrinsics are for allocas. And when we move them into the frame, the original lifetime intrinsics are naturally meaningless. To me, I feel it is meaningless to talk about whether or not the undefined intrinsics are correct or not.
Backport 038dc2c
Requested by: @ChuanqiXu9