[llvm] d216607 - [Coroutine] Split PHI Nodes in `cleanuppad` blocks in a way that obeys EH pad rules
Xun Li via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 25 11:30:48 PDT 2020
Author: Daniel Paoliello
Date: 2020-09-25T11:30:38-07:00
New Revision: d2166076b882e38becf3657ea830ffd2b6a5695e
URL: https://github.com/llvm/llvm-project/commit/d2166076b882e38becf3657ea830ffd2b6a5695e
DIFF: https://github.com/llvm/llvm-project/commit/d2166076b882e38becf3657ea830ffd2b6a5695e.diff
LOG: [Coroutine] Split PHI Nodes in `cleanuppad` blocks in a way that obeys EH pad rules
Issue Details:
In order to support coroutine splitting, any multi-value PHI node in a coroutine is split into multiple blocks with single-value PHI Nodes, which then allows a subsequent transform to generate `reload` instructions as required (i.e., to reload the value if required if the coroutine has been resumed). This causes issues with EH pads (`catchswitch` and `catchpad`) as all pads within a `catchswitch` must have the same unwind destination, but the coroutine splitting logic may modify them to each have a unique unwind destination if there is a PHI node in the unwind `cleanuppad` that is set from values in the `catchswitch` and `cleanuppad` blocks.
Fix Details:
During splitting, if such a PHI node is detected, then create a "dispatcher" `cleanuppad` as well as the blocks with single-value PHI Nodes: thus the "dispatcher" is the unwind destination and it will detect which predecessor called it and then branch to the appropriate single-value PHI node block, which will then branch back to the original `cleanuppad` block.
Reviewed By: GorNishanov, lxfind
Differential Revision: https://reviews.llvm.org/D88059
Added:
llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
Modified:
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 04afd6fe4f54..60681999eea7 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1052,15 +1052,14 @@ static void setUnwindEdgeTo(Instruction *TI, BasicBlock *Succ) {
// Replaces all uses of OldPred with the NewPred block in all PHINodes in a
// block.
static void updatePhiNodes(BasicBlock *DestBB, BasicBlock *OldPred,
- BasicBlock *NewPred,
- PHINode *LandingPadReplacement) {
+ BasicBlock *NewPred, PHINode *Until = nullptr) {
unsigned BBIdx = 0;
for (BasicBlock::iterator I = DestBB->begin(); isa<PHINode>(I); ++I) {
PHINode *PN = cast<PHINode>(I);
// We manually update the LandingPadReplacement PHINode and it is the last
// PHI Node. So, if we find it, we are done.
- if (LandingPadReplacement == PN)
+ if (Until == PN)
break;
// Reuse the previous value of BBIdx if it lines up. In cases where we
@@ -1109,6 +1108,102 @@ static BasicBlock *ehAwareSplitEdge(BasicBlock *BB, BasicBlock *Succ,
return NewBB;
}
+// Moves the values in the PHIs in SuccBB that correspong to PredBB into a new
+// PHI in InsertedBB.
+static void movePHIValuesToInsertedBlock(BasicBlock *SuccBB,
+ BasicBlock *InsertedBB,
+ BasicBlock *PredBB,
+ PHINode *UntilPHI = nullptr) {
+ auto *PN = cast<PHINode>(&SuccBB->front());
+ do {
+ int Index = PN->getBasicBlockIndex(InsertedBB);
+ Value *V = PN->getIncomingValue(Index);
+ PHINode *InputV = PHINode::Create(
+ V->getType(), 1, V->getName() + Twine(".") + SuccBB->getName(),
+ &InsertedBB->front());
+ InputV->addIncoming(V, PredBB);
+ PN->setIncomingValue(Index, InputV);
+ PN = dyn_cast<PHINode>(PN->getNextNode());
+ } while (PN != UntilPHI);
+}
+
+// Rewrites the PHI Nodes in a cleanuppad.
+static void rewritePHIsForCleanupPad(BasicBlock *CleanupPadBB,
+ CleanupPadInst *CleanupPad) {
+ // For every incoming edge to a CleanupPad we will create a new block holding
+ // all incoming values in single-value PHI nodes. We will then create another
+ // block to act as a dispather (as all unwind edges for related EH blocks
+ // must be the same).
+ //
+ // cleanuppad:
+ // %2 = phi i32[%0, %catchswitch], [%1, %catch.1]
+ // %3 = cleanuppad within none []
+ //
+ // It will create:
+ //
+ // cleanuppad.corodispatch
+ // %2 = phi i8[0, %catchswitch], [1, %catch.1]
+ // %3 = cleanuppad within none []
+ // switch i8 % 2, label %unreachable
+ // [i8 0, label %cleanuppad.from.catchswitch
+ // i8 1, label %cleanuppad.from.catch.1]
+ // cleanuppad.from.catchswitch:
+ // %4 = phi i32 [%0, %catchswitch]
+ // br %label cleanuppad
+ // cleanuppad.from.catch.1:
+ // %6 = phi i32 [%1, %catch.1]
+ // br %label cleanuppad
+ // cleanuppad:
+ // %8 = phi i32 [%4, %cleanuppad.from.catchswitch],
+ // [%6, %cleanuppad.from.catch.1]
+
+ // Unreachable BB, in case switching on an invalid value in the dispatcher.
+ auto *UnreachBB = BasicBlock::Create(
+ CleanupPadBB->getContext(), "unreachable", CleanupPadBB->getParent());
+ IRBuilder<> Builder(UnreachBB);
+ Builder.CreateUnreachable();
+
+ // Create a new cleanuppad which will be the dispatcher.
+ auto *NewCleanupPadBB =
+ BasicBlock::Create(CleanupPadBB->getContext(),
+ CleanupPadBB->getName() + Twine(".corodispatch"),
+ CleanupPadBB->getParent(), CleanupPadBB);
+ Builder.SetInsertPoint(NewCleanupPadBB);
+ auto *SwitchType = Builder.getInt8Ty();
+ auto *SetDispatchValuePN =
+ Builder.CreatePHI(SwitchType, pred_size(CleanupPadBB));
+ CleanupPad->removeFromParent();
+ CleanupPad->insertAfter(SetDispatchValuePN);
+ auto *SwitchOnDispatch = Builder.CreateSwitch(SetDispatchValuePN, UnreachBB,
+ pred_size(CleanupPadBB));
+
+ int SwitchIndex = 0;
+ SmallVector<BasicBlock *, 8> Preds(pred_begin(CleanupPadBB),
+ pred_end(CleanupPadBB));
+ for (BasicBlock *Pred : Preds) {
+ // Create a new cleanuppad and move the PHI values to there.
+ auto *CaseBB = BasicBlock::Create(CleanupPadBB->getContext(),
+ CleanupPadBB->getName() +
+ Twine(".from.") + Pred->getName(),
+ CleanupPadBB->getParent(), CleanupPadBB);
+ updatePhiNodes(CleanupPadBB, Pred, CaseBB);
+ CaseBB->setName(CleanupPadBB->getName() + Twine(".from.") +
+ Pred->getName());
+ Builder.SetInsertPoint(CaseBB);
+ Builder.CreateBr(CleanupPadBB);
+ movePHIValuesToInsertedBlock(CleanupPadBB, CaseBB, NewCleanupPadBB);
+
+ // Update this Pred to the new unwind point.
+ setUnwindEdgeTo(Pred->getTerminator(), NewCleanupPadBB);
+
+ // Setup the switch in the dispatcher.
+ auto *SwitchConstant = ConstantInt::get(SwitchType, SwitchIndex);
+ SetDispatchValuePN->addIncoming(SwitchConstant, Pred);
+ SwitchOnDispatch->addCase(SwitchConstant, CaseBB);
+ SwitchIndex++;
+ }
+}
+
static void rewritePHIs(BasicBlock &BB) {
// For every incoming edge we will create a block holding all
// incoming values in a single PHI nodes.
@@ -1131,6 +1226,23 @@ static void rewritePHIs(BasicBlock &BB) {
// TODO: Simplify PHINodes in the basic block to remove duplicate
// predecessors.
+ // Special case for CleanupPad: all EH blocks must have the same unwind edge
+ // so we need to create an additional "dispatcher" block.
+ if (auto *CleanupPad =
+ dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHI())) {
+ SmallVector<BasicBlock *, 8> Preds(pred_begin(&BB), pred_end(&BB));
+ for (BasicBlock *Pred : Preds) {
+ if (CatchSwitchInst *CS =
+ dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
+ // CleanupPad with a CatchSwitch predecessor: therefore this is an
+ // unwind destination that needs to be handle specially.
+ assert(CS->getUnwindDest() == &BB);
+ rewritePHIsForCleanupPad(&BB, CleanupPad);
+ return;
+ }
+ }
+ }
+
LandingPadInst *LandingPad = nullptr;
PHINode *ReplPHI = nullptr;
if ((LandingPad = dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHI()))) {
@@ -1148,18 +1260,10 @@ static void rewritePHIs(BasicBlock &BB) {
for (BasicBlock *Pred : Preds) {
auto *IncomingBB = ehAwareSplitEdge(Pred, &BB, LandingPad, ReplPHI);
IncomingBB->setName(BB.getName() + Twine(".from.") + Pred->getName());
- auto *PN = cast<PHINode>(&BB.front());
- do {
- int Index = PN->getBasicBlockIndex(IncomingBB);
- Value *V = PN->getIncomingValue(Index);
- PHINode *InputV = PHINode::Create(
- V->getType(), 1, V->getName() + Twine(".") + BB.getName(),
- &IncomingBB->front());
- InputV->addIncoming(V, Pred);
- PN->setIncomingValue(Index, InputV);
- PN = dyn_cast<PHINode>(PN->getNextNode());
- } while (PN != ReplPHI); // ReplPHI is either null or the PHI that replaced
- // the landing pad.
+
+ // Stop the moving of values at ReplPHI, as this is either null or the PHI
+ // that replaced the landing pad.
+ movePHIValuesToInsertedBlock(&BB, IncomingBB, Pred, ReplPHI);
}
if (LandingPad) {
diff --git a/llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll b/llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
new file mode 100644
index 000000000000..aa4fdab0a5c2
--- /dev/null
+++ b/llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll
@@ -0,0 +1,117 @@
+; Tests the PHI nodes in cleanuppads for catchswitch instructions are correctly
+; split up.
+; RUN: opt < %s -coro-split -S | FileCheck %s
+; RUN: opt < %s -passes=coro-split -S | FileCheck %s
+
+declare i32 @__CxxFrameHandler3(...)
+define i8* @f2(i1 %val) "coroutine.presplit"="1" personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+ %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
+ %valueA = call i32 @f();
+ %valueB = call i32 @f();
+ %need.alloc = call i1 @llvm.coro.alloc(token %id)
+ br i1 %need.alloc, label %dyn.alloc, label %dowork.0
+
+dyn.alloc:
+ %size = call i32 @llvm.coro.size.i32()
+ %alloc = call i8* @malloc(i32 %size)
+ br label %dowork.0
+
+dowork.0:
+ %phi = phi i8* [ null, %entry ], [ %alloc, %dyn.alloc ]
+ %hdl = call i8* @llvm.coro.begin(token %id, i8* %phi)
+ invoke void @print(i32 0)
+ to label %checksuspend unwind label %catch.dispatch.1
+
+checksuspend:
+ %0 = call i8 @llvm.coro.suspend(token none, i1 false)
+ switch i8 %0, label %suspend [i8 0, label %dowork.1
+ i8 1, label %cleanup]
+
+dowork.1:
+ invoke void @print(i32 0)
+ to label %checksuspend unwind label %catch.dispatch.1
+
+cleanup:
+ %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
+ call void @free(i8* %mem)
+ br label %suspend
+
+suspend:
+ call i1 @llvm.coro.end(i8* %hdl, i1 0)
+ ret i8* %hdl
+
+catch.dispatch.1:
+ %cs1 = catchswitch within none [label %handler1] unwind to caller
+handler1:
+ %h1 = catchpad within %cs1 [i8* null, i32 64, i8* null]
+ invoke void @print(i32 2) [ "funclet"(token %h1) ]
+ to label %catchret1 unwind label %catch.dispatch.2
+catchret1:
+ catchret from %h1 to label %cleanup
+
+catch.dispatch.2:
+ %cs2 = catchswitch within %h1 [label %handler2] unwind label %cleanup2
+handler2:
+ %h2 = catchpad within %cs2 [i8* null, i32 64, i8* null]
+ invoke void @print(i32 3) [ "funclet"(token %h2) ]
+ to label %cleanup unwind label %cleanup2
+cleanup2:
+ %cleanupval2 = phi i32 [%valueA, %catch.dispatch.2], [%valueB, %handler2]
+ cleanuppad within %h1 []
+ call void @print(i32 %cleanupval2)
+ br label %cleanup
+
+; Verifiers that a "dispatcher" cleanuppad is created.
+
+; catchswitch and all associated catchpads are required to have the same unwind
+; edge, but coro requires that PHI nodes are split up so that reload
+; instructions can be generated, therefore we create a new "dispatcher"
+; cleanuppad which forwards to individual blocks that contain the reload
+; instructions per catchswitch/catchpad and then all branch back to the
+; original cleanuppad block.
+
+; CHECK: catch.dispatch.2:
+; CHECK: %cs2 = catchswitch within %h1 [label %handler2] unwind label %cleanup2.corodispatch
+
+; CHECK: handler2:
+; CHECK: invoke void @print(i32 3)
+; CHECK: to label %cleanup unwind label %cleanup2.corodispatch
+
+; CHECK: cleanup2.corodispatch:
+; CHECK: %1 = phi i8 [ 0, %handler2 ], [ 1, %catch.dispatch.2 ]
+; CHECK: %2 = cleanuppad within %h1 []
+; CHECK: %switch = icmp ult i8 %1, 1
+; CHECK: br i1 %switch, label %cleanup2.from.handler2, label %cleanup2.from.catch.dispatch.2
+
+; CHECK: cleanup2.from.handler2:
+; CHECK: %valueB.reload = load i32, i32* %valueB.spill.addr, align 4
+; CHECK: br label %cleanup2
+
+; CHECK: cleanup2.from.catch.dispatch.2:
+; CHECK: %valueA.reload = load i32, i32* %valueA.spill.addr, align 4
+; CHECK: br label %cleanup2
+
+; CHECK: cleanup2:
+; CHECK: %cleanupval2 = phi i32 [ %valueA.reload, %cleanup2.from.catch.dispatch.2 ], [ %valueB.reload, %cleanup2.from.handler2 ]
+; CHECK: call void @print(i32 %cleanupval2)
+; CHECK: br label %cleanup
+}
+
+declare i8* @llvm.coro.free(token, i8*)
+declare i32 @llvm.coro.size.i32()
+declare i8 @llvm.coro.suspend(token, i1)
+declare void @llvm.coro.resume(i8*)
+declare void @llvm.coro.destroy(i8*)
+
+declare token @llvm.coro.id(i32, i8*, i8*, i8*)
+declare i1 @llvm.coro.alloc(token)
+declare i8* @llvm.coro.begin(token, i8*)
+declare i1 @llvm.coro.end(i8*, i1)
+
+declare noalias i8* @malloc(i32)
+declare void @print(i32)
+declare void @free(i8*)
+
+declare i32 @f()
+
More information about the llvm-commits
mailing list