[llvm] r357007 - [WebAssembly] Fix bugs in BLOCK/TRY placement
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 26 10:15:56 PDT 2019
Author: aheejin
Date: Tue Mar 26 10:15:55 2019
New Revision: 357007
URL: http://llvm.org/viewvc/llvm-project?rev=357007&view=rev
Log:
[WebAssembly] Fix bugs in BLOCK/TRY placement
Summary:
Before we placed all TRY/END_TRY markers before placing BLOCK/END_BLOCK
markers. This couldn't handle this case:
```
bb0:
br bb2
bb1: // nearest common dominator of bb3 and bb4
br_if ... bb3
br bb4
bb2:
...
bb3:
call @foo // unwinds to ehpad
bb4:
call @bar // unwinds to ehpad
ehpad:
catch
...
```
When we placed TRY markers, we placed it in bb1 because it is the
nearest common dominator of bb3 and bb4. But because bb0 jumps to bb2,
when we placed block markers, we ended up with interleaved scopes like
```
block
try
end_block
catch
end_try
```
which was not correct.
This patch fixes the bug by placing BLOCK and TRY markers in one pass
while iterating BBs in a function. This also adds some more routines to
`placeTryMarkers`, because we now have to assume that there can be
previously placed BLOCK and END_BLOCK.
Reviewers: dschuff
Subscribers: sunfish, sbc100, jgravelle-google, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D59739
Modified:
llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
llvm/trunk/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
Modified: llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp?rev=357007&r1=357006&r2=357007&view=diff
==============================================================================
--- llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp (original)
+++ llvm/trunk/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp Tue Mar 26 10:15:55 2019
@@ -193,10 +193,7 @@ void WebAssemblyCFGStackify::unregisterS
/// Insert a BLOCK marker for branches to MBB (if needed).
void WebAssemblyCFGStackify::placeBlockMarker(MachineBasicBlock &MBB) {
- // This should have been handled in placeTryMarker.
- if (MBB.isEHPad())
- return;
-
+ assert(!MBB.isEHPad());
MachineFunction &MF = *MBB.getParent();
auto &MDT = getAnalysis<MachineDominatorTree>();
const auto &TII = *MF.getSubtarget<WebAssemblySubtarget>().getInstrInfo();
@@ -253,14 +250,12 @@ void WebAssemblyCFGStackify::placeBlockM
// Instructions that should go after the BLOCK.
SmallPtrSet<const MachineInstr *, 4> AfterSet;
for (const auto &MI : *Header) {
- // If there is a previously placed LOOP/TRY marker and the bottom block of
- // the loop/exception is above MBB, it should be after the BLOCK, because
- // the loop/exception is nested in this block. Otherwise it should be before
- // the BLOCK.
- if (MI.getOpcode() == WebAssembly::LOOP ||
- MI.getOpcode() == WebAssembly::TRY) {
- auto *BottomBB = BeginToEnd[&MI]->getParent()->getPrevNode();
- if (MBB.getNumber() > BottomBB->getNumber())
+ // If there is a previously placed LOOP marker and the bottom block of the
+ // loop is above MBB, it should be after the BLOCK, because the loop is
+ // nested in this BLOCK. Otherwise it should be before the BLOCK.
+ if (MI.getOpcode() == WebAssembly::LOOP) {
+ auto *LoopBottom = BeginToEnd[&MI]->getParent()->getPrevNode();
+ if (MBB.getNumber() > LoopBottom->getNumber())
AfterSet.insert(&MI);
#ifndef NDEBUG
else
@@ -268,9 +263,10 @@ void WebAssemblyCFGStackify::placeBlockM
#endif
}
- // All previously inserted BLOCK markers should be after the BLOCK because
- // they are all nested blocks.
- if (MI.getOpcode() == WebAssembly::BLOCK)
+ // All previously inserted BLOCK/TRY markers should be after the BLOCK
+ // because they are all nested blocks.
+ if (MI.getOpcode() == WebAssembly::BLOCK ||
+ MI.getOpcode() == WebAssembly::TRY)
AfterSet.insert(&MI);
#ifndef NDEBUG
@@ -428,9 +424,7 @@ void WebAssemblyCFGStackify::placeLoopMa
}
void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {
- if (!MBB.isEHPad())
- return;
-
+ assert(MBB.isEHPad());
MachineFunction &MF = *MBB.getParent();
auto &MDT = getAnalysis<MachineDominatorTree>();
const auto &TII = *MF.getSubtarget<WebAssemblySubtarget>().getInstrInfo();
@@ -486,16 +480,17 @@ void WebAssemblyCFGStackify::placeTryMar
// Decide where in Header to put the TRY.
- // Instructions that should go before the BLOCK.
+ // Instructions that should go before the TRY.
SmallPtrSet<const MachineInstr *, 4> BeforeSet;
- // Instructions that should go after the BLOCK.
+ // Instructions that should go after the TRY.
SmallPtrSet<const MachineInstr *, 4> AfterSet;
for (const auto &MI : *Header) {
- // If there is a previously placed LOOP marker and the bottom block of
- // the loop is above MBB, the LOOP should be after the TRY, because the
- // loop is nested in this try. Otherwise it should be before the TRY.
+ // If there is a previously placed LOOP marker and the bottom block of the
+ // loop is above MBB, it should be after the TRY, because the loop is nested
+ // in this TRY. Otherwise it should be before the TRY.
if (MI.getOpcode() == WebAssembly::LOOP) {
- if (MBB.getNumber() > Bottom->getNumber())
+ auto *LoopBottom = BeginToEnd[&MI]->getParent()->getPrevNode();
+ if (MBB.getNumber() > LoopBottom->getNumber())
AfterSet.insert(&MI);
#ifndef NDEBUG
else
@@ -503,14 +498,16 @@ void WebAssemblyCFGStackify::placeTryMar
#endif
}
- // All previously inserted TRY markers should be after the TRY because they
- // are all nested trys.
- if (MI.getOpcode() == WebAssembly::TRY)
+ // All previously inserted BLOCK/TRY markers should be after the TRY because
+ // they are all nested trys.
+ if (MI.getOpcode() == WebAssembly::BLOCK ||
+ MI.getOpcode() == WebAssembly::TRY)
AfterSet.insert(&MI);
#ifndef NDEBUG
- // All END_(LOOP/TRY) markers should be before the TRY.
- if (MI.getOpcode() == WebAssembly::END_LOOP ||
+ // All END_(BLOCK/LOOP/TRY) markers should be before the TRY.
+ if (MI.getOpcode() == WebAssembly::END_BLOCK ||
+ MI.getOpcode() == WebAssembly::END_LOOP ||
MI.getOpcode() == WebAssembly::END_TRY)
BeforeSet.insert(&MI);
#endif
@@ -566,8 +563,9 @@ void WebAssemblyCFGStackify::placeTryMar
AfterSet.clear();
for (const auto &MI : *Cont) {
#ifndef NDEBUG
- // END_TRY should precede existing LOOP markers.
- if (MI.getOpcode() == WebAssembly::LOOP)
+ // END_TRY should precede existing LOOP and BLOCK markers.
+ if (MI.getOpcode() == WebAssembly::LOOP ||
+ MI.getOpcode() == WebAssembly::BLOCK)
AfterSet.insert(&MI);
// All END_TRY markers placed earlier belong to exceptions that contains
@@ -588,6 +586,8 @@ void WebAssemblyCFGStackify::placeTryMar
AfterSet.insert(&MI);
#endif
}
+
+ // It is not possible for an END_BLOCK to be already in this block.
}
// Mark the end of the TRY.
@@ -774,15 +774,19 @@ void WebAssemblyCFGStackify::placeMarker
// Place the LOOP for MBB if MBB is the header of a loop.
for (auto &MBB : MF)
placeLoopMarker(MBB);
- // Place the TRY for MBB if MBB is the EH pad of an exception.
+
const MCAsmInfo *MCAI = MF.getTarget().getMCAsmInfo();
- if (MCAI->getExceptionHandlingType() == ExceptionHandling::Wasm &&
- MF.getFunction().hasPersonalityFn())
- for (auto &MBB : MF)
- placeTryMarker(MBB);
- // Place the BLOCK for MBB if MBB is branched to from above.
- for (auto &MBB : MF)
- placeBlockMarker(MBB);
+ for (auto &MBB : MF) {
+ if (MBB.isEHPad()) {
+ // Place the TRY for MBB if MBB is the EH pad of an exception.
+ if (MCAI->getExceptionHandlingType() == ExceptionHandling::Wasm &&
+ MF.getFunction().hasPersonalityFn())
+ placeTryMarker(MBB);
+ } else {
+ // Place the BLOCK for MBB if MBB is branched to from above.
+ placeBlockMarker(MBB);
+ }
+ }
}
void WebAssemblyCFGStackify::rewriteDepthImmediates(MachineFunction &MF) {
Modified: llvm/trunk/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/WebAssembly/cfg-stackify-eh.ll?rev=357007&r1=357006&r2=357007&view=diff
==============================================================================
--- llvm/trunk/test/CodeGen/WebAssembly/cfg-stackify-eh.ll (original)
+++ llvm/trunk/test/CodeGen/WebAssembly/cfg-stackify-eh.ll Tue Mar 26 10:15:55 2019
@@ -1,4 +1,5 @@
; RUN: llc < %s -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling | FileCheck %s
+; RUN: llc < %s -O0 -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -verify-machineinstrs -exception-model=wasm -mattr=+exception-handling | FileCheck %s --check-prefix=NOOPT
target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
target triple = "wasm32-unknown-unknown"
@@ -70,7 +71,7 @@ rethrow:
call void @llvm.wasm.rethrow.in.catch() [ "funclet"(token %1) ]
unreachable
-try.cont: ; preds = %entry, %catch, %catch2
+try.cont: ; preds = %catch, %catch2, %entry
ret void
}
@@ -173,7 +174,7 @@ rethrow5:
invoke void @llvm.wasm.rethrow.in.catch() [ "funclet"(token %9) ]
to label %unreachable unwind label %ehcleanup9
-try.cont: ; preds = %catch, %invoke.cont8
+try.cont: ; preds = %invoke.cont8, %catch
call void @__cxa_end_catch() [ "funclet"(token %1) ]
catchret from %1 to label %try.cont11
@@ -181,7 +182,7 @@ rethrow:
call void @llvm.wasm.rethrow.in.catch() [ "funclet"(token %1) ]
unreachable
-try.cont11: ; preds = %entry, %try.cont
+try.cont11: ; preds = %try.cont, %entry
ret void
ehcleanup: ; preds = %catch6
@@ -286,6 +287,54 @@ terminate:
unreachable
}
+; Tests if block and try markers are correctly placed. Even if two predecessors
+; of the EH pad are bb2 and bb3 and their nearest common dominator is bb1, the
+; TRY marker should be placed at bb0 because there's a branch from bb0 to bb2,
+; and scopes cannot be interleaved.
+
+; NOOPT-LABEL: test3
+; NOOPT: try
+; NOOPT: block
+; NOOPT: block
+; NOOPT: block
+; NOOPT: end_block
+; NOOPT: end_block
+; NOOPT: call foo
+; NOOPT: end_block
+; NOOPT: call bar
+; NOOPT: catch {{.*}}
+; NOOPT: end_try
+define void @test3() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+bb0:
+ br i1 undef, label %bb1, label %bb2
+
+bb1: ; preds = %bb0
+ br i1 undef, label %bb3, label %bb4
+
+bb2: ; preds = %bb0
+ br label %try.cont
+
+bb3: ; preds = %bb1
+ invoke void @foo()
+ to label %try.cont unwind label %catch.dispatch
+
+bb4: ; preds = %bb1
+ invoke void @bar()
+ to label %try.cont unwind label %catch.dispatch
+
+catch.dispatch: ; preds = %bb4, %bb3
+ %0 = catchswitch within none [label %catch.start] unwind to caller
+
+catch.start: ; preds = %catch.dispatch
+ %1 = catchpad within %0 [i8* null]
+ %2 = call i8* @llvm.wasm.get.exception(token %1)
+ %3 = call i32 @llvm.wasm.get.ehselector(token %1)
+ catchret from %1 to label %try.cont
+
+try.cont: ; preds = %catch.start, %bb4, %bb3, %bb2
+ ret void
+}
+
declare void @foo()
declare void @bar()
declare i32 @__gxx_wasm_personality_v0(...)
More information about the llvm-commits
mailing list