Skip to content

Commit d216607

Browse files
dpaoliellolxfind
dpaoliello
authored and
lxfind
committed
[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
1 parent 2c2a749 commit d216607

File tree

2 files changed

+236
-15
lines changed

2 files changed

+236
-15
lines changed

‎llvm/lib/Transforms/Coroutines/CoroFrame.cpp

Lines changed: 119 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,15 +1052,14 @@ static void setUnwindEdgeTo(Instruction *TI, BasicBlock *Succ) {
10521052
// Replaces all uses of OldPred with the NewPred block in all PHINodes in a
10531053
// block.
10541054
static void updatePhiNodes(BasicBlock *DestBB, BasicBlock *OldPred,
1055-
BasicBlock *NewPred,
1056-
PHINode *LandingPadReplacement) {
1055+
BasicBlock *NewPred, PHINode *Until = nullptr) {
10571056
unsigned BBIdx = 0;
10581057
for (BasicBlock::iterator I = DestBB->begin(); isa<PHINode>(I); ++I) {
10591058
PHINode *PN = cast<PHINode>(I);
10601059

10611060
// We manually update the LandingPadReplacement PHINode and it is the last
10621061
// PHI Node. So, if we find it, we are done.
1063-
if (LandingPadReplacement == PN)
1062+
if (Until == PN)
10641063
break;
10651064

10661065
// 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,
11091108
return NewBB;
11101109
}
11111110

1111+
// Moves the values in the PHIs in SuccBB that correspong to PredBB into a new
1112+
// PHI in InsertedBB.
1113+
static void movePHIValuesToInsertedBlock(BasicBlock *SuccBB,
1114+
BasicBlock *InsertedBB,
1115+
BasicBlock *PredBB,
1116+
PHINode *UntilPHI = nullptr) {
1117+
auto *PN = cast<PHINode>(&SuccBB->front());
1118+
do {
1119+
int Index = PN->getBasicBlockIndex(InsertedBB);
1120+
Value *V = PN->getIncomingValue(Index);
1121+
PHINode *InputV = PHINode::Create(
1122+
V->getType(), 1, V->getName() + Twine(".") + SuccBB->getName(),
1123+
&InsertedBB->front());
1124+
InputV->addIncoming(V, PredBB);
1125+
PN->setIncomingValue(Index, InputV);
1126+
PN = dyn_cast<PHINode>(PN->getNextNode());
1127+
} while (PN != UntilPHI);
1128+
}
1129+
1130+
// Rewrites the PHI Nodes in a cleanuppad.
1131+
static void rewritePHIsForCleanupPad(BasicBlock *CleanupPadBB,
1132+
CleanupPadInst *CleanupPad) {
1133+
// For every incoming edge to a CleanupPad we will create a new block holding
1134+
// all incoming values in single-value PHI nodes. We will then create another
1135+
// block to act as a dispather (as all unwind edges for related EH blocks
1136+
// must be the same).
1137+
//
1138+
// cleanuppad:
1139+
// %2 = phi i32[%0, %catchswitch], [%1, %catch.1]
1140+
// %3 = cleanuppad within none []
1141+
//
1142+
// It will create:
1143+
//
1144+
// cleanuppad.corodispatch
1145+
// %2 = phi i8[0, %catchswitch], [1, %catch.1]
1146+
// %3 = cleanuppad within none []
1147+
// switch i8 % 2, label %unreachable
1148+
// [i8 0, label %cleanuppad.from.catchswitch
1149+
// i8 1, label %cleanuppad.from.catch.1]
1150+
// cleanuppad.from.catchswitch:
1151+
// %4 = phi i32 [%0, %catchswitch]
1152+
// br %label cleanuppad
1153+
// cleanuppad.from.catch.1:
1154+
// %6 = phi i32 [%1, %catch.1]
1155+
// br %label cleanuppad
1156+
// cleanuppad:
1157+
// %8 = phi i32 [%4, %cleanuppad.from.catchswitch],
1158+
// [%6, %cleanuppad.from.catch.1]
1159+
1160+
// Unreachable BB, in case switching on an invalid value in the dispatcher.
1161+
auto *UnreachBB = BasicBlock::Create(
1162+
CleanupPadBB->getContext(), "unreachable", CleanupPadBB->getParent());
1163+
IRBuilder<> Builder(UnreachBB);
1164+
Builder.CreateUnreachable();
1165+
1166+
// Create a new cleanuppad which will be the dispatcher.
1167+
auto *NewCleanupPadBB =
1168+
BasicBlock::Create(CleanupPadBB->getContext(),
1169+
CleanupPadBB->getName() + Twine(".corodispatch"),
1170+
CleanupPadBB->getParent(), CleanupPadBB);
1171+
Builder.SetInsertPoint(NewCleanupPadBB);
1172+
auto *SwitchType = Builder.getInt8Ty();
1173+
auto *SetDispatchValuePN =
1174+
Builder.CreatePHI(SwitchType, pred_size(CleanupPadBB));
1175+
CleanupPad->removeFromParent();
1176+
CleanupPad->insertAfter(SetDispatchValuePN);
1177+
auto *SwitchOnDispatch = Builder.CreateSwitch(SetDispatchValuePN, UnreachBB,
1178+
pred_size(CleanupPadBB));
1179+
1180+
int SwitchIndex = 0;
1181+
SmallVector<BasicBlock *, 8> Preds(pred_begin(CleanupPadBB),
1182+
pred_end(CleanupPadBB));
1183+
for (BasicBlock *Pred : Preds) {
1184+
// Create a new cleanuppad and move the PHI values to there.
1185+
auto *CaseBB = BasicBlock::Create(CleanupPadBB->getContext(),
1186+
CleanupPadBB->getName() +
1187+
Twine(".from.") + Pred->getName(),
1188+
CleanupPadBB->getParent(), CleanupPadBB);
1189+
updatePhiNodes(CleanupPadBB, Pred, CaseBB);
1190+
CaseBB->setName(CleanupPadBB->getName() + Twine(".from.") +
1191+
Pred->getName());
1192+
Builder.SetInsertPoint(CaseBB);
1193+
Builder.CreateBr(CleanupPadBB);
1194+
movePHIValuesToInsertedBlock(CleanupPadBB, CaseBB, NewCleanupPadBB);
1195+
1196+
// Update this Pred to the new unwind point.
1197+
setUnwindEdgeTo(Pred->getTerminator(), NewCleanupPadBB);
1198+
1199+
// Setup the switch in the dispatcher.
1200+
auto *SwitchConstant = ConstantInt::get(SwitchType, SwitchIndex);
1201+
SetDispatchValuePN->addIncoming(SwitchConstant, Pred);
1202+
SwitchOnDispatch->addCase(SwitchConstant, CaseBB);
1203+
SwitchIndex++;
1204+
}
1205+
}
1206+
11121207
static void rewritePHIs(BasicBlock &BB) {
11131208
// For every incoming edge we will create a block holding all
11141209
// incoming values in a single PHI nodes.
@@ -1131,6 +1226,23 @@ static void rewritePHIs(BasicBlock &BB) {
11311226
// TODO: Simplify PHINodes in the basic block to remove duplicate
11321227
// predecessors.
11331228

1229+
// Special case for CleanupPad: all EH blocks must have the same unwind edge
1230+
// so we need to create an additional "dispatcher" block.
1231+
if (auto *CleanupPad =
1232+
dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHI())) {
1233+
SmallVector<BasicBlock *, 8> Preds(pred_begin(&BB), pred_end(&BB));
1234+
for (BasicBlock *Pred : Preds) {
1235+
if (CatchSwitchInst *CS =
1236+
dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
1237+
// CleanupPad with a CatchSwitch predecessor: therefore this is an
1238+
// unwind destination that needs to be handle specially.
1239+
assert(CS->getUnwindDest() == &BB);
1240+
rewritePHIsForCleanupPad(&BB, CleanupPad);
1241+
return;
1242+
}
1243+
}
1244+
}
1245+
11341246
LandingPadInst *LandingPad = nullptr;
11351247
PHINode *ReplPHI = nullptr;
11361248
if ((LandingPad = dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHI()))) {
@@ -1148,18 +1260,10 @@ static void rewritePHIs(BasicBlock &BB) {
11481260
for (BasicBlock *Pred : Preds) {
11491261
auto *IncomingBB = ehAwareSplitEdge(Pred, &BB, LandingPad, ReplPHI);
11501262
IncomingBB->setName(BB.getName() + Twine(".from.") + Pred->getName());
1151-
auto *PN = cast<PHINode>(&BB.front());
1152-
do {
1153-
int Index = PN->getBasicBlockIndex(IncomingBB);
1154-
Value *V = PN->getIncomingValue(Index);
1155-
PHINode *InputV = PHINode::Create(
1156-
V->getType(), 1, V->getName() + Twine(".") + BB.getName(),
1157-
&IncomingBB->front());
1158-
InputV->addIncoming(V, Pred);
1159-
PN->setIncomingValue(Index, InputV);
1160-
PN = dyn_cast<PHINode>(PN->getNextNode());
1161-
} while (PN != ReplPHI); // ReplPHI is either null or the PHI that replaced
1162-
// the landing pad.
1263+
1264+
// Stop the moving of values at ReplPHI, as this is either null or the PHI
1265+
// that replaced the landing pad.
1266+
movePHIValuesToInsertedBlock(&BB, IncomingBB, Pred, ReplPHI);
11631267
}
11641268

11651269
if (LandingPad) {

‎llvm/test/Transforms/Coroutines/coro-catchswitch-cleanuppad.ll

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
; Tests the PHI nodes in cleanuppads for catchswitch instructions are correctly
2+
; split up.
3+
; RUN: opt < %s -coro-split -S | FileCheck %s
4+
; RUN: opt < %s -passes=coro-split -S | FileCheck %s
5+
6+
declare i32 @__CxxFrameHandler3(...)
7+
define i8* @f2(i1 %val) "coroutine.presplit"="1" personality i32 (...)* @__CxxFrameHandler3 {
8+
entry:
9+
%id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
10+
%valueA = call i32 @f();
11+
%valueB = call i32 @f();
12+
%need.alloc = call i1 @llvm.coro.alloc(token %id)
13+
br i1 %need.alloc, label %dyn.alloc, label %dowork.0
14+
15+
dyn.alloc:
16+
%size = call i32 @llvm.coro.size.i32()
17+
%alloc = call i8* @malloc(i32 %size)
18+
br label %dowork.0
19+
20+
dowork.0:
21+
%phi = phi i8* [ null, %entry ], [ %alloc, %dyn.alloc ]
22+
%hdl = call i8* @llvm.coro.begin(token %id, i8* %phi)
23+
invoke void @print(i32 0)
24+
to label %checksuspend unwind label %catch.dispatch.1
25+
26+
checksuspend:
27+
%0 = call i8 @llvm.coro.suspend(token none, i1 false)
28+
switch i8 %0, label %suspend [i8 0, label %dowork.1
29+
i8 1, label %cleanup]
30+
31+
dowork.1:
32+
invoke void @print(i32 0)
33+
to label %checksuspend unwind label %catch.dispatch.1
34+
35+
cleanup:
36+
%mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
37+
call void @free(i8* %mem)
38+
br label %suspend
39+
40+
suspend:
41+
call i1 @llvm.coro.end(i8* %hdl, i1 0)
42+
ret i8* %hdl
43+
44+
catch.dispatch.1:
45+
%cs1 = catchswitch within none [label %handler1] unwind to caller
46+
handler1:
47+
%h1 = catchpad within %cs1 [i8* null, i32 64, i8* null]
48+
invoke void @print(i32 2) [ "funclet"(token %h1) ]
49+
to label %catchret1 unwind label %catch.dispatch.2
50+
catchret1:
51+
catchret from %h1 to label %cleanup
52+
53+
catch.dispatch.2:
54+
%cs2 = catchswitch within %h1 [label %handler2] unwind label %cleanup2
55+
handler2:
56+
%h2 = catchpad within %cs2 [i8* null, i32 64, i8* null]
57+
invoke void @print(i32 3) [ "funclet"(token %h2) ]
58+
to label %cleanup unwind label %cleanup2
59+
cleanup2:
60+
%cleanupval2 = phi i32 [%valueA, %catch.dispatch.2], [%valueB, %handler2]
61+
cleanuppad within %h1 []
62+
call void @print(i32 %cleanupval2)
63+
br label %cleanup
64+
65+
; Verifiers that a "dispatcher" cleanuppad is created.
66+
67+
; catchswitch and all associated catchpads are required to have the same unwind
68+
; edge, but coro requires that PHI nodes are split up so that reload
69+
; instructions can be generated, therefore we create a new "dispatcher"
70+
; cleanuppad which forwards to individual blocks that contain the reload
71+
; instructions per catchswitch/catchpad and then all branch back to the
72+
; original cleanuppad block.
73+
74+
; CHECK: catch.dispatch.2:
75+
; CHECK: %cs2 = catchswitch within %h1 [label %handler2] unwind label %cleanup2.corodispatch
76+
77+
; CHECK: handler2:
78+
; CHECK: invoke void @print(i32 3)
79+
; CHECK: to label %cleanup unwind label %cleanup2.corodispatch
80+
81+
; CHECK: cleanup2.corodispatch:
82+
; CHECK: %1 = phi i8 [ 0, %handler2 ], [ 1, %catch.dispatch.2 ]
83+
; CHECK: %2 = cleanuppad within %h1 []
84+
; CHECK: %switch = icmp ult i8 %1, 1
85+
; CHECK: br i1 %switch, label %cleanup2.from.handler2, label %cleanup2.from.catch.dispatch.2
86+
87+
; CHECK: cleanup2.from.handler2:
88+
; CHECK: %valueB.reload = load i32, i32* %valueB.spill.addr, align 4
89+
; CHECK: br label %cleanup2
90+
91+
; CHECK: cleanup2.from.catch.dispatch.2:
92+
; CHECK: %valueA.reload = load i32, i32* %valueA.spill.addr, align 4
93+
; CHECK: br label %cleanup2
94+
95+
; CHECK: cleanup2:
96+
; CHECK: %cleanupval2 = phi i32 [ %valueA.reload, %cleanup2.from.catch.dispatch.2 ], [ %valueB.reload, %cleanup2.from.handler2 ]
97+
; CHECK: call void @print(i32 %cleanupval2)
98+
; CHECK: br label %cleanup
99+
}
100+
101+
declare i8* @llvm.coro.free(token, i8*)
102+
declare i32 @llvm.coro.size.i32()
103+
declare i8 @llvm.coro.suspend(token, i1)
104+
declare void @llvm.coro.resume(i8*)
105+
declare void @llvm.coro.destroy(i8*)
106+
107+
declare token @llvm.coro.id(i32, i8*, i8*, i8*)
108+
declare i1 @llvm.coro.alloc(token)
109+
declare i8* @llvm.coro.begin(token, i8*)
110+
declare i1 @llvm.coro.end(i8*, i1)
111+
112+
declare noalias i8* @malloc(i32)
113+
declare void @print(i32)
114+
declare void @free(i8*)
115+
116+
declare i32 @f()
117+

0 commit comments

Comments
 (0)