Skip to content

Commit 0a47e8c

Browse files
spaitsspaits
authored
Reland [BasicBlockUtils] Handle funclets when detaching EH pad blocks (#159379)
Fixes #148052 . Last PR did not account for the scenario, when more than one instruction used the `catchpad` label. In that case I have deleted uses, which were already "choosen to be iterated over" by the early increment iterator. This issue was not visible in normal release build on x86, but luckily later on the address sanitizer build it has found it on the buildbot. Here is the diff from the last version of this PR: #158435 ```diff diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp index 91e245e..1dd8cb4 100644 --- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp +++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp @@ -106,7 +106,8 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs, // first block, the we would have possible cleanupret and catchret // instructions with poison arguments, which wouldn't be valid. if (isa<FuncletPadInst>(I)) { - for (User *User : make_early_inc_range(I.users())) { + SmallPtrSet<BasicBlock *, 4> UniqueEHRetBlocksToDelete; + for (User *User : I.users()) { Instruction *ReturnInstr = dyn_cast<Instruction>(User); // If we have a cleanupret or catchret block, replace it with just an // unreachable. The other alternative, that may use a catchpad is a @@ -114,33 +115,12 @@ void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs, if (isa<CatchReturnInst>(ReturnInstr) || isa<CleanupReturnInst>(ReturnInstr)) { BasicBlock *ReturnInstrBB = ReturnInstr->getParent(); - // This catchret or catchpad basic block is detached now. Let the - // successors know it. - // This basic block also may have some predecessors too. For - // example the following LLVM-IR is valid: - // - // [cleanuppad_block] - // | - // [regular_block] - // | - // [cleanupret_block] - // - // The IR after the cleanup will look like this: - // - // [cleanuppad_block] - // | - // [regular_block] - // | - // [unreachable] - // - // So regular_block will lead to an unreachable block, which is also - // valid. There is no need to replace regular_block with unreachable - // in this context now. - // On the other hand, the cleanupret/catchret block's successors - // need to know about the deletion of their predecessors. - emptyAndDetachBlock(ReturnInstrBB, Updates, KeepOneInputPHIs); + UniqueEHRetBlocksToDelete.insert(ReturnInstrBB); } } + for (BasicBlock *EHRetBB : + make_early_inc_range(UniqueEHRetBlocksToDelete)) + emptyAndDetachBlock(EHRetBB, Updates, KeepOneInputPHIs); } } ```
1 parent a38794f commit 0a47e8c

File tree

2 files changed

+305
-28
lines changed

2 files changed

+305
-28
lines changed

‎llvm/lib/Transforms/Utils/BasicBlockUtils.cpp‎

Lines changed: 69 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -58,37 +58,78 @@ static cl::opt<unsigned> MaxDeoptOrUnreachableSuccessorCheckDepth(
5858
"is followed by a block that either has a terminating "
5959
"deoptimizing call or is terminated with an unreachable"));
6060

61-
void llvm::detachDeadBlocks(
62-
ArrayRef<BasicBlock *> BBs,
63-
SmallVectorImpl<DominatorTree::UpdateType> *Updates,
64-
bool KeepOneInputPHIs) {
61+
/// Zap all the instructions in the block and replace them with an unreachable
62+
/// instruction and notify the basic block's successors that one of their
63+
/// predecessors is going away.
64+
static void
65+
emptyAndDetachBlock(BasicBlock *BB,
66+
SmallVectorImpl<DominatorTree::UpdateType> *Updates,
67+
bool KeepOneInputPHIs) {
68+
// Loop through all of our successors and make sure they know that one
69+
// of their predecessors is going away.
70+
SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
71+
for (BasicBlock *Succ : successors(BB)) {
72+
Succ->removePredecessor(BB, KeepOneInputPHIs);
73+
if (Updates && UniqueSuccessors.insert(Succ).second)
74+
Updates->push_back({DominatorTree::Delete, BB, Succ});
75+
}
76+
77+
// Zap all the instructions in the block.
78+
while (!BB->empty()) {
79+
Instruction &I = BB->back();
80+
// If this instruction is used, replace uses with an arbitrary value.
81+
// Because control flow can't get here, we don't care what we replace the
82+
// value with. Note that since this block is unreachable, and all values
83+
// contained within it must dominate their uses, that all uses will
84+
// eventually be removed (they are themselves dead).
85+
if (!I.use_empty())
86+
I.replaceAllUsesWith(PoisonValue::get(I.getType()));
87+
BB->back().eraseFromParent();
88+
}
89+
new UnreachableInst(BB->getContext(), BB);
90+
assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
91+
"The successor list of BB isn't empty before "
92+
"applying corresponding DTU updates.");
93+
}
94+
95+
void llvm::detachDeadBlocks(ArrayRef<BasicBlock *> BBs,
96+
SmallVectorImpl<DominatorTree::UpdateType> *Updates,
97+
bool KeepOneInputPHIs) {
98+
SmallPtrSet<BasicBlock *, 4> UniqueEHRetBlocksToDelete;
6599
for (auto *BB : BBs) {
66-
// Loop through all of our successors and make sure they know that one
67-
// of their predecessors is going away.
68-
SmallPtrSet<BasicBlock *, 4> UniqueSuccessors;
69-
for (BasicBlock *Succ : successors(BB)) {
70-
Succ->removePredecessor(BB, KeepOneInputPHIs);
71-
if (Updates && UniqueSuccessors.insert(Succ).second)
72-
Updates->push_back({DominatorTree::Delete, BB, Succ});
73-
}
100+
auto NonFirstPhiIt = BB->getFirstNonPHIIt();
101+
if (NonFirstPhiIt != BB->end()) {
102+
Instruction &I = *NonFirstPhiIt;
103+
// Exception handling funclets need to be explicitly addressed.
104+
// These funclets must begin with cleanuppad or catchpad and end with
105+
// cleanupred or catchret. The return instructions can be in different
106+
// basic blocks than the pad instruction. If we would only delete the
107+
// first block, the we would have possible cleanupret and catchret
108+
// instructions with poison arguments, which wouldn't be valid.
109+
if (isa<FuncletPadInst>(I)) {
110+
UniqueEHRetBlocksToDelete.clear();
111+
112+
for (User *User : I.users()) {
113+
Instruction *ReturnInstr = dyn_cast<Instruction>(User);
114+
// If we have a cleanupret or catchret block, replace it with just an
115+
// unreachable. The other alternative, that may use a catchpad is a
116+
// catchswitch. That does not need special handling for now.
117+
if (isa<CatchReturnInst>(ReturnInstr) ||
118+
isa<CleanupReturnInst>(ReturnInstr)) {
119+
BasicBlock *ReturnInstrBB = ReturnInstr->getParent();
120+
UniqueEHRetBlocksToDelete.insert(ReturnInstrBB);
121+
}
122+
}
74123

75-
// Zap all the instructions in the block.
76-
while (!BB->empty()) {
77-
Instruction &I = BB->back();
78-
// If this instruction is used, replace uses with an arbitrary value.
79-
// Because control flow can't get here, we don't care what we replace the
80-
// value with. Note that since this block is unreachable, and all values
81-
// contained within it must dominate their uses, that all uses will
82-
// eventually be removed (they are themselves dead).
83-
if (!I.use_empty())
84-
I.replaceAllUsesWith(PoisonValue::get(I.getType()));
85-
BB->back().eraseFromParent();
124+
for (BasicBlock *EHRetBB : UniqueEHRetBlocksToDelete)
125+
emptyAndDetachBlock(EHRetBB, Updates, KeepOneInputPHIs);
126+
}
86127
}
87-
new UnreachableInst(BB->getContext(), BB);
88-
assert(BB->size() == 1 &&
89-
isa<UnreachableInst>(BB->getTerminator()) &&
90-
"The successor list of BB isn't empty before "
91-
"applying corresponding DTU updates.");
128+
129+
UniqueEHRetBlocksToDelete.clear();
130+
131+
// Detaching and emptying the current basic block.
132+
emptyAndDetachBlock(BB, Updates, KeepOneInputPHIs);
92133
}
93134
}
94135

‎llvm/test/Transforms/SimplifyCFG/unreachable-multi-basic-block-funclet.ll‎

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
2+
; RUN: opt -passes=simplifycfg -S < %s | FileCheck %s
3+
4+
; cleanuppad/cleanupret
5+
6+
define void @unreachable_cleanuppad_linear(i64 %shapes.1) personality ptr null {
7+
; CHECK-LABEL: define void @unreachable_cleanuppad_linear(
8+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
9+
; CHECK-NEXT: [[START:.*:]]
10+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
11+
; CHECK-NEXT: ret void
12+
;
13+
start:
14+
%_7 = icmp ult i64 0, %shapes.1
15+
ret void
16+
17+
funclet:
18+
%cleanuppad = cleanuppad within none []
19+
br label %funclet_end
20+
21+
funclet_end:
22+
cleanupret from %cleanuppad unwind to caller
23+
}
24+
25+
define void @unreachable_cleanuppad_linear_middle_block(i64 %shapes.1) personality ptr null {
26+
; CHECK-LABEL: define void @unreachable_cleanuppad_linear_middle_block(
27+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
28+
; CHECK-NEXT: [[START:.*:]]
29+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
30+
; CHECK-NEXT: ret void
31+
;
32+
start:
33+
%_7 = icmp ult i64 0, %shapes.1
34+
ret void
35+
36+
funclet:
37+
%cleanuppad = cleanuppad within none []
38+
br label %middle_block
39+
40+
middle_block:
41+
%tmp1 = add i64 %shapes.1, 42
42+
br label %funclet_end
43+
44+
funclet_end:
45+
cleanupret from %cleanuppad unwind to caller
46+
}
47+
48+
define void @unreachable_cleanuppad_multiple_predecessors(i64 %shapes.1) personality ptr null {
49+
; CHECK-LABEL: define void @unreachable_cleanuppad_multiple_predecessors(
50+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
51+
; CHECK-NEXT: [[START:.*:]]
52+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
53+
; CHECK-NEXT: ret void
54+
;
55+
start:
56+
%_7 = icmp ult i64 0, %shapes.1
57+
ret void
58+
59+
funclet:
60+
%cleanuppad = cleanuppad within none []
61+
switch i64 %shapes.1, label %otherwise [ i64 0, label %one
62+
i64 1, label %two
63+
i64 42, label %three ]
64+
one:
65+
br label %funclet_end
66+
67+
two:
68+
br label %funclet_end
69+
70+
three:
71+
br label %funclet_end
72+
73+
otherwise:
74+
br label %funclet_end
75+
76+
funclet_end:
77+
cleanupret from %cleanuppad unwind to caller
78+
}
79+
80+
; catchpad/catchret
81+
82+
define void @unreachable_catchpad_linear(i64 %shapes.1) personality ptr null {
83+
; CHECK-LABEL: define void @unreachable_catchpad_linear(
84+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
85+
; CHECK-NEXT: [[START:.*:]]
86+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
87+
; CHECK-NEXT: ret void
88+
;
89+
start:
90+
%_7 = icmp ult i64 0, %shapes.1
91+
ret void
92+
93+
dispatch:
94+
%cs = catchswitch within none [label %funclet] unwind to caller
95+
96+
funclet:
97+
%cleanuppad = catchpad within %cs []
98+
br label %funclet_end
99+
100+
101+
funclet_end:
102+
catchret from %cleanuppad to label %unreachable
103+
104+
unreachable:
105+
unreachable
106+
}
107+
108+
define void @unreachable_catchpad_multiple_predecessors(i64 %shapes.1) personality ptr null {
109+
; CHECK-LABEL: define void @unreachable_catchpad_multiple_predecessors(
110+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
111+
; CHECK-NEXT: [[START:.*:]]
112+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
113+
; CHECK-NEXT: ret void
114+
;
115+
start:
116+
%_7 = icmp ult i64 0, %shapes.1
117+
ret void
118+
119+
dispatch:
120+
%cs = catchswitch within none [label %funclet] unwind to caller
121+
122+
funclet:
123+
%cleanuppad = catchpad within %cs []
124+
switch i64 %shapes.1, label %otherwise [ i64 0, label %one
125+
i64 1, label %two
126+
i64 42, label %three ]
127+
one:
128+
br label %funclet_end
129+
130+
two:
131+
br label %funclet_end
132+
133+
three:
134+
br label %funclet_end
135+
136+
otherwise:
137+
br label %funclet_end
138+
139+
funclet_end:
140+
catchret from %cleanuppad to label %unreachable
141+
142+
unreachable:
143+
unreachable
144+
}
145+
146+
; Issue reproducer
147+
148+
define void @gh148052(i64 %shapes.1) personality ptr null {
149+
; CHECK-LABEL: define void @gh148052(
150+
; CHECK-SAME: i64 [[SHAPES_1:%.*]]) personality ptr null {
151+
; CHECK-NEXT: [[START:.*:]]
152+
; CHECK-NEXT: [[_7:%.*]] = icmp ult i64 0, [[SHAPES_1]]
153+
; CHECK-NEXT: call void @llvm.assume(i1 [[_7]])
154+
; CHECK-NEXT: ret void
155+
;
156+
start:
157+
%_7 = icmp ult i64 0, %shapes.1
158+
br i1 %_7, label %bb1, label %panic
159+
160+
bb1:
161+
%_11 = icmp ult i64 0, %shapes.1
162+
br i1 %_11, label %bb3, label %panic1
163+
164+
panic:
165+
unreachable
166+
167+
bb3:
168+
ret void
169+
170+
panic1:
171+
invoke void @func(i64 0, i64 0, ptr null)
172+
to label %unreachable unwind label %funclet_bb14
173+
174+
funclet_bb14:
175+
%cleanuppad = cleanuppad within none []
176+
br label %bb13
177+
178+
unreachable:
179+
unreachable
180+
181+
bb10:
182+
cleanupret from %cleanuppad5 unwind to caller
183+
184+
funclet_bb10:
185+
%cleanuppad5 = cleanuppad within none []
186+
br label %bb10
187+
188+
bb13:
189+
cleanupret from %cleanuppad unwind label %funclet_bb10
190+
}
191+
192+
%struct.foo = type { ptr, %struct.eggs, ptr }
193+
%struct.eggs = type { ptr, ptr, ptr }
194+
195+
declare x86_thiscallcc ptr @quux(ptr, ptr, i32)
196+
197+
define x86_thiscallcc ptr @baz(ptr %arg, ptr %arg1, ptr %arg2, i1 %arg3, ptr %arg4) personality ptr null {
198+
; CHECK-LABEL: define x86_thiscallcc ptr @baz(
199+
; CHECK-SAME: ptr [[ARG:%.*]], ptr [[ARG1:%.*]], ptr [[ARG2:%.*]], i1 [[ARG3:%.*]], ptr [[ARG4:%.*]]) personality ptr null {
200+
; CHECK-NEXT: [[BB:.*:]]
201+
; CHECK-NEXT: [[ALLOCA:%.*]] = alloca [2 x %struct.foo], align 4
202+
; CHECK-NEXT: [[INVOKE:%.*]] = call x86_thiscallcc ptr @quux(ptr null, ptr null, i32 0) #[[ATTR1:[0-9]+]]
203+
; CHECK-NEXT: unreachable
204+
;
205+
bb:
206+
%alloca = alloca [2 x %struct.foo], align 4
207+
%invoke = invoke x86_thiscallcc ptr @quux(ptr null, ptr null, i32 0)
208+
to label %bb5 unwind label %bb10
209+
210+
bb5: ; preds = %bb
211+
%getelementptr = getelementptr i8, ptr %arg, i32 20
212+
%call = call x86_thiscallcc ptr null(ptr null, ptr null, i32 0)
213+
br label %bb6
214+
215+
bb6: ; preds = %bb10, %bb5
216+
%phi = phi ptr [ null, %bb10 ], [ null, %bb5 ]
217+
ret ptr %phi
218+
219+
bb7: ; No predecessors!
220+
%cleanuppad = cleanuppad within none []
221+
%getelementptr8 = getelementptr i8, ptr %arg2, i32 -20
222+
%icmp = icmp eq ptr %arg, null
223+
br label %bb9
224+
225+
bb9: ; preds = %bb7
226+
cleanupret from %cleanuppad unwind label %bb10
227+
228+
bb10: ; preds = %bb9, %bb
229+
%phi11 = phi ptr [ %arg, %bb9 ], [ null, %bb ]
230+
%cleanuppad12 = cleanuppad within none []
231+
%getelementptr13 = getelementptr i8, ptr %phi11, i32 -20
232+
store i32 0, ptr %phi11, align 4
233+
br label %bb6
234+
}
235+
236+
declare void @func(i64, i64, ptr)

0 commit comments

Comments
 (0)