[WebAssembly] Treat 'rethrow' as terminator in custom isel #95967
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`rethrow` instruction is a terminator, but when when its DAG is built in `SelectionDAGBuilder` in a custom routine, it was NOT treated as such. ```ll rethrow: ; preds = %catch.start invoke void @llvm.wasm.rethrow() llvm#1 [ "funclet"(token %1) ] to label %unreachable unwind label %ehcleanup ehcleanup: ; preds = %rethrow, %catch.dispatch %tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ] ... ``` In this bitcode, because of the `phi`, a `CONST_I32` will be created in the `rethrow` BB. Without this patch, the DAG for the `rethrow` BB looks like this: ``` t0: ch,glue = EntryToken t3: ch = CopyToReg t0, Register:i32 %9, Constant:i32<20> t5: ch = llvm.wasm.rethrow t0, TargetConstant:i32<12161> t6: ch = TokenFactor t3, t5 t8: ch = br t6, BasicBlock:ch<unreachable 0x562532e43c50> ``` Note that `CopyToReg` and `llvm.wasm.rethrow` don't have dependence so either can come first in the selected code, which can result in the code like ```mir bb.3.rethrow: RETHROW 0, implicit-def dead $arguments %9:i32 = CONST_I32 20, implicit-def dead $arguments BR %bb.6, implicit-def dead $arguments ``` After this patch, `llvm.wasm.rethrow` is treated as a terminator, and the DAG will look like ``` t0: ch,glue = EntryToken t3: ch = CopyToReg t0, Register:i32 %9, Constant:i32<20> t5: ch = llvm.wasm.rethrow t3, TargetConstant:i32<12161> t7: ch = br t5, BasicBlock:ch<unreachable 0x5555e3d32c70> ``` Note that now `rethrow` takes a token from `CopyToReg`, so `rethrow` has to come after `CopyToReg`. And the resulting code will be ```mir bb.3.rethrow: %9:i32 = CONST_I32 20, implicit-def dead $arguments RETHROW 0, implicit-def dead $arguments BR %bb.6, implicit-def dead $arguments ``` I'm not very familiar with the internals of `getRoot` vs. `getControlRoot`, but other terminator instructions seem to use the latter, and using it for `rethrow` too worked.
|
@llvm/pr-subscribers-llvm-selectiondag Author: Heejin Ahn (aheejin) Changes
rethrow: ; preds = %catch.start
invoke void @<!-- -->llvm.wasm.rethrow() #<!-- -->1 [ "funclet"(token %1) ]
to label %unreachable unwind label %ehcleanup
ehcleanup: ; preds = %rethrow, %catch.dispatch
%tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ]
...In this bitcode, because of the Note that bb.3.rethrow:
RETHROW 0, implicit-def dead $arguments
%9:i32 = CONST_I32 20, implicit-def dead $arguments
BR %bb.6, implicit-def dead $argumentsAfter this patch, Note that now bb.3.rethrow:
%9:i32 = CONST_I32 20, implicit-def dead $arguments
RETHROW 0, implicit-def dead $arguments
BR %bb.6, implicit-def dead $argumentsI'm not very familiar with the internals of Full diff: https://github.com/llvm/llvm-project/pull/95967.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index be5e0f6ef058b..e6578d9f2b140 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3355,7 +3355,7 @@ void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) {
// special because it can be invoked, so we manually lower it to a DAG
// node here.
SmallVector<SDValue, 8> Ops;
- Ops.push_back(getRoot()); // inchain
+ Ops.push_back(getControlRoot()); // inchain for the terminator node
const TargetLowering &TLI = DAG.getTargetLoweringInfo();
Ops.push_back(
DAG.getTargetConstant(Intrinsic::wasm_rethrow, getCurSDLoc(),
diff --git a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
index dfa33f95f37be..3537baa425164 100644
--- a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
+++ b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
@@ -350,8 +350,60 @@ ehcleanupret: ; preds = %catch.start, %ehcle
cleanupret from %0 unwind to caller
}
+; Regression test for the bug that 'rethrow' was not treated correctly as a
+; terminator in isel.
+define void @test_rethrow_terminator() 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 %ehcleanup
+
+catch.start: ; preds = %catch.dispatch
+ %1 = catchpad within %0 [ptr @_ZTIi]
+ %2 = call ptr @llvm.wasm.get.exception(token %1)
+ %3 = call i32 @llvm.wasm.get.ehselector(token %1)
+ %4 = call i32 @llvm.eh.typeid.for.p0(ptr @_ZTIi)
+ %matches = icmp eq i32 %3, %4
+ br i1 %matches, label %catch, label %rethrow
+
+catch: ; preds = %catch.start
+ %5 = call ptr @__cxa_begin_catch(ptr %2) [ "funclet"(token %1) ]
+ %6 = load i32, ptr %5, align 4
+ call void @__cxa_end_catch() [ "funclet"(token %1) ]
+ catchret from %1 to label %try.cont
+
+rethrow: ; preds = %catch.start
+ invoke void @llvm.wasm.rethrow() #1 [ "funclet"(token %1) ]
+ to label %unreachable unwind label %ehcleanup
+
+try.cont: ; preds = %entry, %catch
+ ret void
+
+ehcleanup: ; preds = %rethrow, %catch.dispatch
+ ; 'rethrow' BB is this BB's predecessor, and its
+ ; 'invoke void @llvm.wasm.rethrow()' is lowered down to a 'RETHROW' in Wasm
+ ; MIR. And this 'phi' creates 'CONST_I32' instruction in the predecessor
+ ; 'rethrow' BB. If 'RETHROW' is not treated correctly as a terminator, it can
+ ; create a BB like
+ ; bb.3.rethrow:
+ ; RETHROW 0
+ ; %0 = CONST_I32 20
+ ; BR ...
+ %tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ]
+ %7 = cleanuppad within none []
+ call void @take_i32(i32 %tmp) [ "funclet"(token %7) ]
+ cleanupret from %7 unwind to caller
+
+unreachable: ; preds = %rethrow
+ unreachable
+}
+
+
declare void @foo()
declare void @bar(ptr)
+declare void @take_i32(i32)
declare i32 @__gxx_wasm_personality_v0(...)
; Function Attrs: noreturn
declare void @llvm.wasm.throw(i32, ptr) #1
|
|
@llvm/pr-subscribers-backend-webassembly Author: Heejin Ahn (aheejin) Changes
rethrow: ; preds = %catch.start
invoke void @<!-- -->llvm.wasm.rethrow() #<!-- -->1 [ "funclet"(token %1) ]
to label %unreachable unwind label %ehcleanup
ehcleanup: ; preds = %rethrow, %catch.dispatch
%tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ]
...In this bitcode, because of the Note that bb.3.rethrow:
RETHROW 0, implicit-def dead $arguments
%9:i32 = CONST_I32 20, implicit-def dead $arguments
BR %bb.6, implicit-def dead $argumentsAfter this patch, Note that now bb.3.rethrow:
%9:i32 = CONST_I32 20, implicit-def dead $arguments
RETHROW 0, implicit-def dead $arguments
BR %bb.6, implicit-def dead $argumentsI'm not very familiar with the internals of Full diff: https://github.com/llvm/llvm-project/pull/95967.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index be5e0f6ef058b..e6578d9f2b140 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3355,7 +3355,7 @@ void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) {
// special because it can be invoked, so we manually lower it to a DAG
// node here.
SmallVector<SDValue, 8> Ops;
- Ops.push_back(getRoot()); // inchain
+ Ops.push_back(getControlRoot()); // inchain for the terminator node
const TargetLowering &TLI = DAG.getTargetLoweringInfo();
Ops.push_back(
DAG.getTargetConstant(Intrinsic::wasm_rethrow, getCurSDLoc(),
diff --git a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
index dfa33f95f37be..3537baa425164 100644
--- a/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
+++ b/llvm/test/CodeGen/WebAssembly/exception-legacy.ll
@@ -350,8 +350,60 @@ ehcleanupret: ; preds = %catch.start, %ehcle
cleanupret from %0 unwind to caller
}
+; Regression test for the bug that 'rethrow' was not treated correctly as a
+; terminator in isel.
+define void @test_rethrow_terminator() 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 %ehcleanup
+
+catch.start: ; preds = %catch.dispatch
+ %1 = catchpad within %0 [ptr @_ZTIi]
+ %2 = call ptr @llvm.wasm.get.exception(token %1)
+ %3 = call i32 @llvm.wasm.get.ehselector(token %1)
+ %4 = call i32 @llvm.eh.typeid.for.p0(ptr @_ZTIi)
+ %matches = icmp eq i32 %3, %4
+ br i1 %matches, label %catch, label %rethrow
+
+catch: ; preds = %catch.start
+ %5 = call ptr @__cxa_begin_catch(ptr %2) [ "funclet"(token %1) ]
+ %6 = load i32, ptr %5, align 4
+ call void @__cxa_end_catch() [ "funclet"(token %1) ]
+ catchret from %1 to label %try.cont
+
+rethrow: ; preds = %catch.start
+ invoke void @llvm.wasm.rethrow() #1 [ "funclet"(token %1) ]
+ to label %unreachable unwind label %ehcleanup
+
+try.cont: ; preds = %entry, %catch
+ ret void
+
+ehcleanup: ; preds = %rethrow, %catch.dispatch
+ ; 'rethrow' BB is this BB's predecessor, and its
+ ; 'invoke void @llvm.wasm.rethrow()' is lowered down to a 'RETHROW' in Wasm
+ ; MIR. And this 'phi' creates 'CONST_I32' instruction in the predecessor
+ ; 'rethrow' BB. If 'RETHROW' is not treated correctly as a terminator, it can
+ ; create a BB like
+ ; bb.3.rethrow:
+ ; RETHROW 0
+ ; %0 = CONST_I32 20
+ ; BR ...
+ %tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ]
+ %7 = cleanuppad within none []
+ call void @take_i32(i32 %tmp) [ "funclet"(token %7) ]
+ cleanupret from %7 unwind to caller
+
+unreachable: ; preds = %rethrow
+ unreachable
+}
+
+
declare void @foo()
declare void @bar(ptr)
+declare void @take_i32(i32)
declare i32 @__gxx_wasm_personality_v0(...)
; Function Attrs: noreturn
declare void @llvm.wasm.throw(i32, ptr) #1
|
dschuff
approved these changes
Jun 18, 2024
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
`rethrow` instruction is a terminator, but when when its DAG is built in `SelectionDAGBuilder` in a custom routine, it was NOT treated as such. ```ll rethrow: ; preds = %catch.start invoke void @llvm.wasm.rethrow() llvm#1 [ "funclet"(token %1) ] to label %unreachable unwind label %ehcleanup ehcleanup: ; preds = %rethrow, %catch.dispatch %tmp = phi i32 [ 10, %catch.dispatch ], [ 20, %rethrow ] ... ``` In this bitcode, because of the `phi`, a `CONST_I32` will be created in the `rethrow` BB. Without this patch, the DAG for the `rethrow` BB looks like this: ``` t0: ch,glue = EntryToken t3: ch = CopyToReg t0, Register:i32 %9, Constant:i32<20> t5: ch = llvm.wasm.rethrow t0, TargetConstant:i32<12161> t6: ch = TokenFactor t3, t5 t8: ch = br t6, BasicBlock:ch<unreachable 0x562532e43c50> ``` Note that `CopyToReg` and `llvm.wasm.rethrow` don't have dependence so either can come first in the selected code, which can result in the code like ```mir bb.3.rethrow: RETHROW 0, implicit-def dead $arguments %9:i32 = CONST_I32 20, implicit-def dead $arguments BR %bb.6, implicit-def dead $arguments ``` After this patch, `llvm.wasm.rethrow` is treated as a terminator, and the DAG will look like ``` t0: ch,glue = EntryToken t3: ch = CopyToReg t0, Register:i32 %9, Constant:i32<20> t5: ch = llvm.wasm.rethrow t3, TargetConstant:i32<12161> t7: ch = br t5, BasicBlock:ch<unreachable 0x5555e3d32c70> ``` Note that now `rethrow` takes a token from `CopyToReg`, so `rethrow` has to come after `CopyToReg`. And the resulting code will be ```mir bb.3.rethrow: %9:i32 = CONST_I32 20, implicit-def dead $arguments RETHROW 0, implicit-def dead $arguments BR %bb.6, implicit-def dead $arguments ``` I'm not very familiar with the internals of `getRoot` vs. `getControlRoot`, but other terminator instructions seem to use the latter, and using it for `rethrow` too worked.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rethrowinstruction is a terminator, but when when its DAG is built inSelectionDAGBuilderin a custom routine, it was NOT treated as such.In this bitcode, because of the
phi, aCONST_I32will be created in therethrowBB. Without this patch, the DAG for therethrowBB looks like this:Note that
CopyToRegandllvm.wasm.rethrowdon't have dependence so either can come first in the selected code, which can result in the code likeAfter this patch,
llvm.wasm.rethrowis treated as a terminator, and the DAG will look likeNote that now
rethrowtakes a token fromCopyToReg, sorethrowhas to come afterCopyToReg. And the resulting code will beI'm not very familiar with the internals of
getRootvs.getControlRoot, but other terminator instructions seem to use the latter, and using it forrethrowtoo worked.