Skip to content

[WebAssembly] Remove wasm-specific findWasmUnwindDestinations #130374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 11, 2025
Merged

[WebAssembly] Remove wasm-specific findWasmUnwindDestinations #130374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37a56fc
[WebAssembly] Remove wasm-specific findWasmUnwindDestinations
aheejin Mar 5, 2025
67b7839
Update comment
aheejin Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 5 additions & 61 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2028,58 +2028,6 @@ void SelectionDAGBuilder::visitCleanupPad(const CleanupPadInst &CPI) {
}
}

// In wasm EH, even though a catchpad may not catch an exception if a tag does
// not match, it is OK to add only the first unwind destination catchpad to the
// successors, because there will be at least one invoke instruction within the
// catch scope that points to the next unwind destination, if one exists, so
// CFGSort cannot mess up with BB sorting order.
// (All catchpads with 'catch (type)' clauses have a 'llvm.rethrow' intrinsic
// call within them, and catchpads only consisting of 'catch (...)' have a
// '__cxa_end_catch' call within them, both of which generate invokes in case
// the next unwind destination exists, i.e., the next unwind destination is not
// the caller.)
//
// Having at most one EH pad successor is also simpler and helps later
// transformations.
//
// For example,
// current:
// invoke void @foo to ... unwind label %catch.dispatch
// catch.dispatch:
// %0 = catchswitch within ... [label %catch.start] unwind label %next
// catch.start:
// ...
// ... in this BB or some other child BB dominated by this BB there will be an
// invoke that points to 'next' BB as an unwind destination
//
// next: ; We don't need to add this to 'current' BB's successor
// ...
static void findWasmUnwindDestinations(
FunctionLoweringInfo &FuncInfo, const BasicBlock *EHPadBB,
BranchProbability Prob,
SmallVectorImpl<std::pair<MachineBasicBlock *, BranchProbability>>
&UnwindDests) {
while (EHPadBB) {
BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt();
if (isa<CleanupPadInst>(Pad)) {
// Stop on cleanup pads.
UnwindDests.emplace_back(FuncInfo.getMBB(EHPadBB), Prob);
UnwindDests.back().first->setIsEHScopeEntry();
break;
} else if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(Pad)) {
// Add the catchpad handlers to the possible destinations. We don't
// continue to the unwind destination of the catchswitch for wasm.
for (const BasicBlock *CatchPadBB : CatchSwitch->handlers()) {
UnwindDests.emplace_back(FuncInfo.getMBB(CatchPadBB), Prob);
UnwindDests.back().first->setIsEHScopeEntry();
}
break;
} else {
continue;
}
}
}

/// When an invoke or a cleanupret unwinds to the next EH pad, there are
/// many places it could ultimately go. In the IR, we have a single unwind
/// destination, but in the machine CFG, we enumerate all the possible blocks.
Expand All @@ -2100,13 +2048,6 @@ static void findUnwindDestinations(
bool IsWasmCXX = Personality == EHPersonality::Wasm_CXX;
bool IsSEH = isAsynchronousEHPersonality(Personality);

if (IsWasmCXX) {
findWasmUnwindDestinations(FuncInfo, EHPadBB, Prob, UnwindDests);
assert(UnwindDests.size() <= 1 &&
"There should be at most one unwind destination for wasm");
return;
}

while (EHPadBB) {
BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt();
BasicBlock *NewEHPadBB = nullptr;
Expand All @@ -2116,10 +2057,13 @@ static void findUnwindDestinations(
break;
} else if (isa<CleanupPadInst>(Pad)) {
// Stop on cleanup pads. Cleanups are always funclet entries for all known
// personalities.
// personalities except Wasm. And in Wasm this becomes a catch_all(_ref),
// which always catches an exception.
UnwindDests.emplace_back(FuncInfo.getMBB(EHPadBB), Prob);
UnwindDests.back().first->setIsEHScopeEntry();
UnwindDests.back().first->setIsEHFuncletEntry();
// In Wasm, EH scopes are not funclets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the comment on line 2059 above should be updated too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!IsWasmCXX)
UnwindDests.back().first->setIsEHFuncletEntry();
break;
} else if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(Pad)) {
// Add the catchpad handlers to the possible destinations.
Expand Down
20 changes: 17 additions & 3 deletions llvm/lib/CodeGen/WinEHPrepare.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,16 +866,30 @@ void WinEHPrepareImpl::demotePHIsOnFunclets(Function &F,
for (BasicBlock &BB : make_early_inc_range(F)) {
if (!BB.isEHPad())
continue;
if (DemoteCatchSwitchPHIOnly &&
!isa<CatchSwitchInst>(BB.getFirstNonPHIIt()))
continue;

for (Instruction &I : make_early_inc_range(BB)) {
auto *PN = dyn_cast<PHINode>(&I);
// Stop at the first non-PHI.
if (!PN)
break;

// If DemoteCatchSwitchPHIOnly is true, we only demote a PHI when
// 1. The PHI is within a catchswitch BB
// 2. The PHI has a catchswitch BB has one of its incoming blocks
if (DemoteCatchSwitchPHIOnly) {
bool IsCatchSwitchBB = isa<CatchSwitchInst>(BB.getFirstNonPHIIt());
bool HasIncomingCatchSwitchBB = false;
for (unsigned I = 0, E = PN->getNumIncomingValues(); I < E; ++I) {
if (isa<CatchSwitchInst>(
PN->getIncomingBlock(I)->getFirstNonPHIIt())) {
HasIncomingCatchSwitchBB = true;
break;
}
}
if (!IsCatchSwitchBB && !HasIncomingCatchSwitchBB)
break;
}

AllocaInst *SpillSlot = insertPHILoads(PN, F);
if (SpillSlot)
insertPHIStores(PN, SpillSlot);
Expand Down
9 changes: 4 additions & 5 deletions llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1850,13 +1850,12 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {

// If the EH pad on the stack top is where this instruction should unwind
// next, we're good.
MachineBasicBlock *UnwindDest = getFakeCallerBlock(MF);
MachineBasicBlock *UnwindDest = nullptr;
Copy link
Member Author

@aheejin aheejin Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by NFC fix. Fake caller block is used not here but somewhere below, and this part of the code cannot be entered when there is no EH pad successor:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp

Lines 1846 to 1848 in ab87206

if (SeenThrowableInstInBB || !MBB.hasEHPadSuccessor() ||
!WebAssembly::mayThrow(MI))
continue;

which will be assigned to UnwindDest below, so this has no meaning other than creating a may-not-be-necessary BB.

for (auto *Succ : MBB.successors()) {
// Even though semantically a BB can have multiple successors in case an
// exception is not caught by a catchpad, in our backend implementation
// it is guaranteed that a BB can have at most one EH pad successor. For
// details, refer to comments in findWasmUnwindDestinations function in
// SelectionDAGBuilder.cpp.
// exception is not caught by a catchpad, the first unwind destination
// should appear first in the successor list, based on the calculation
// in findUnwindDestinations() in SelectionDAGBuilder.cpp.
if (Succ->isEHPad()) {
UnwindDest = Succ;
break;
Expand Down
3 changes: 0 additions & 3 deletions llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,9 +659,6 @@ static bool canLongjmp(const Value *Callee) {
// Every catchpad generated by Wasm C++ contains __cxa_end_catch, so we
// intentionally treat it as longjmpable to work around this problem. This is
// a hacky fix but an easy one.
//
// The comment block in findWasmUnwindDestinations() in
// SelectionDAGBuilder.cpp is addressing a similar problem.
if (CalleeName == "__cxa_end_catch")
return WebAssembly::WasmEnableSjLj;
if (CalleeName == "__cxa_begin_catch" ||
Expand Down
49 changes: 47 additions & 2 deletions llvm/test/CodeGen/WebAssembly/exception-legacy.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ invoke.cont2: ; preds = %ehcleanup

terminate: ; preds = %ehcleanup
%6 = cleanuppad within %5 []
call void @_ZSt9terminatev() [ "funclet"(token %6) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %6) ]
unreachable
}

Expand Down Expand Up @@ -477,7 +477,7 @@ invoke.cont.i: ; preds = %catch.start.i

terminate.i: ; preds = %catch.start.i, %catch.dispatch.i
%6 = cleanuppad within %0 []
call void @_ZSt9terminatev() [ "funclet"(token %6) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %6) ]
unreachable

_ZN4TempD2Ev.exit: ; preds = %invoke.cont.i
Expand All @@ -501,6 +501,50 @@ unreachable: ; preds = %entry
unreachable
}

; Regression test for a bug where, when an invoke unwinds to a catchswitch, the
; catchswitch's unwind destination was not included in the invoke's unwind
; destination when there was no direct link from catch.start to there.

; CHECK-LABEL: unwind_destinations:
; CHECK: try
; CHECK: try
; CHECK: call foo
; CHECK: catch $0=, __cpp_exception
; CHECK: call _ZSt9terminatev
; CHECK: unreachable
; CHECK: end_try
; Note the below is "terminate" BB and should not be DCE'd
; CHECK: catch_all
; CHECK: call _ZSt9terminatev
; CHECK: unreachable
; CHECK: end_try
; CHECK: return
define void @unwind_destinations() personality ptr @__gxx_wasm_personality_v0 {
entry:
invoke void @foo()
to label %try.cont unwind label %catch.dispatch

catch.dispatch: ; preds = %entry
%0 = catchswitch within none [label %catch.start] unwind label %terminate

catch.start: ; preds = %catch.dispatch
%1 = catchpad within %0 [ptr null]
%2 = call ptr @llvm.wasm.get.exception(token %1)
%3 = call ptr @__cxa_begin_catch(ptr %2) #5 [ "funclet"(token %1) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %1) ]
unreachable

; Even if there is no link from catch.start to this terminate BB, when there is
; an exception that catch.start does not catch (e.g. a foreign exception), it
; should end up here, so this BB should NOT be DCE'ed
terminate: ; preds = %catch.dispatch
%4 = cleanuppad within none []
call void @_ZSt9terminatev() #2 [ "funclet"(token %4) ]
unreachable

try.cont: ; preds = %entry
ret void
}

declare void @foo()
declare void @bar(ptr)
Expand All @@ -527,5 +571,6 @@ declare ptr @_ZN4TempD2Ev(ptr returned)

attributes #0 = { nounwind }
attributes #1 = { noreturn }
attributes #2 = { noreturn nounwind }

; CHECK: __cpp_exception:
55 changes: 53 additions & 2 deletions llvm/test/CodeGen/WebAssembly/exception.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ invoke.cont2: ; preds = %ehcleanup

terminate: ; preds = %ehcleanup
%6 = cleanuppad within %5 []
call void @_ZSt9terminatev() [ "funclet"(token %6) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %6) ]
unreachable
}

Expand Down Expand Up @@ -542,7 +542,7 @@ invoke.cont.i: ; preds = %catch.start.i

terminate.i: ; preds = %catch.start.i, %catch.dispatch.i
%6 = cleanuppad within %0 []
call void @_ZSt9terminatev() [ "funclet"(token %6) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %6) ]
unreachable

_ZN4TempD2Ev.exit: ; preds = %invoke.cont.i
Expand Down Expand Up @@ -593,6 +593,56 @@ try.cont: ; preds = %catch, %entry
ret void
}

; Regression test for a bug where, when an invoke unwinds to a catchswitch, the
; catchswitch's unwind destination was not included in the invoke's unwind
; destination when there was no direct link from catch.start to there.

; CHECK-LABEL: unwind_destinations:
; CHECK: block
; CHECK: block
; CHECK: try_table (catch_all 0)
; CHECK: block i32
; CHECK: try_table (catch __cpp_exception 0)
; CHECK: call foo
; CHECK: end_try_table
; CHECK: unreachable
; CHECK: end_block
; CHECK: call _ZSt9terminatev
; CHECK: unreachable
; CHECK: end_try_table
; CHECK: unreachable
; CHECK: end_block
; Note the below is "terminate" BB and should not be DCE'd
; CHECK: call _ZSt9terminatev
; CHECK: unreachable
; CHECK: end_block
define void @unwind_destinations() personality ptr @__gxx_wasm_personality_v0 {
entry:
invoke void @foo()
to label %try.cont unwind label %catch.dispatch

catch.dispatch: ; preds = %entry
%0 = catchswitch within none [label %catch.start] unwind label %terminate

catch.start: ; preds = %catch.dispatch
%1 = catchpad within %0 [ptr null]
%2 = call ptr @llvm.wasm.get.exception(token %1)
%3 = call ptr @__cxa_begin_catch(ptr %2) #5 [ "funclet"(token %1) ]
call void @_ZSt9terminatev() #2 [ "funclet"(token %1) ]
unreachable

; Even if there is no link from catch.start to this terminate BB, when there is
; an exception that catch.start does not catch (e.g. a foreign exception), it
; should end up here, so this BB should NOT be DCE'ed
terminate: ; preds = %catch.dispatch
%4 = cleanuppad within none []
call void @_ZSt9terminatev() #2 [ "funclet"(token %4) ]
unreachable

try.cont: ; preds = %entry
ret void
}

declare void @foo()
declare void @bar(ptr)
declare void @take_i32(i32)
Expand All @@ -618,5 +668,6 @@ declare ptr @_ZN4TempD2Ev(ptr returned)

attributes #0 = { nounwind }
attributes #1 = { noreturn }
attributes #2 = { noreturn nounwind }

; CHECK: __cpp_exception:
Loading