diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp index d5a07e616236e..53779594b0ce7 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp @@ -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> - &UnwindDests) { - while (EHPadBB) { - BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt(); - if (isa(Pad)) { - // Stop on cleanup pads. - UnwindDests.emplace_back(FuncInfo.getMBB(EHPadBB), Prob); - UnwindDests.back().first->setIsEHScopeEntry(); - break; - } else if (const auto *CatchSwitch = dyn_cast(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. @@ -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; @@ -2116,10 +2057,13 @@ static void findUnwindDestinations( break; } else if (isa(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 + if (!IsWasmCXX) + UnwindDests.back().first->setIsEHFuncletEntry(); break; } else if (const auto *CatchSwitch = dyn_cast(Pad)) { // Add the catchpad handlers to the possible destinations. diff --git a/llvm/lib/CodeGen/WinEHPrepare.cpp b/llvm/lib/CodeGen/WinEHPrepare.cpp index 1970716485613..de351903efa79 100644 --- a/llvm/lib/CodeGen/WinEHPrepare.cpp +++ b/llvm/lib/CodeGen/WinEHPrepare.cpp @@ -866,9 +866,6 @@ void WinEHPrepareImpl::demotePHIsOnFunclets(Function &F, for (BasicBlock &BB : make_early_inc_range(F)) { if (!BB.isEHPad()) continue; - if (DemoteCatchSwitchPHIOnly && - !isa(BB.getFirstNonPHIIt())) - continue; for (Instruction &I : make_early_inc_range(BB)) { auto *PN = dyn_cast(&I); @@ -876,6 +873,23 @@ void WinEHPrepareImpl::demotePHIsOnFunclets(Function &F, 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(BB.getFirstNonPHIIt()); + bool HasIncomingCatchSwitchBB = false; + for (unsigned I = 0, E = PN->getNumIncomingValues(); I < E; ++I) { + if (isa( + PN->getIncomingBlock(I)->getFirstNonPHIIt())) { + HasIncomingCatchSwitchBB = true; + break; + } + } + if (!IsCatchSwitchBB && !HasIncomingCatchSwitchBB) + break; + } + AllocaInst *SpillSlot = insertPHILoads(PN, F); if (SpillSlot) insertPHIStores(PN, SpillSlot); diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp index 277d353d1db10..640be5fe8e8c9 100644 --- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp +++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp @@ -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; 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; diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp index 550d8b64dca35..2d11019291a7a 100644 --- a/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp +++ b/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp @@ -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" || diff --git a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll index 3fe45bcc4cd29..b4ffd185e3ca8 100644 --- a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll +++ b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll @@ -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 } @@ -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 @@ -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) @@ -527,5 +571,6 @@ declare ptr @_ZN4TempD2Ev(ptr returned) attributes #0 = { nounwind } attributes #1 = { noreturn } +attributes #2 = { noreturn nounwind } ; CHECK: __cpp_exception: diff --git a/llvm/test/CodeGen/WebAssembly/exception.ll b/llvm/test/CodeGen/WebAssembly/exception.ll index 57d1f37c0039f..fc42f050a687e 100644 --- a/llvm/test/CodeGen/WebAssembly/exception.ll +++ b/llvm/test/CodeGen/WebAssembly/exception.ll @@ -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 } @@ -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 @@ -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) @@ -618,5 +668,6 @@ declare ptr @_ZN4TempD2Ev(ptr returned) attributes #0 = { nounwind } attributes #1 = { noreturn } +attributes #2 = { noreturn nounwind } ; CHECK: __cpp_exception: