[PATCH] D57134: [WebAssembly] Exception handling: Switch to the new proposal
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 28 14:33:06 PST 2019
aheejin marked 10 inline comments as done.
aheejin added inline comments.
================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:252
+ ExtractExnF =
+ Intrinsic::getDeclaration(&M, Intrinsic::wasm_extract_exception);
+
----------------
dschuff wrote:
> aheejin wrote:
> > If an intrinsic has a 'token' argument, it does not go through isel well.
> For windows, do those just get lowered away in EHPrepare too then?
`wasm.get.exception` intrinsic is only used by wasm so it doesn't matter. But there are other intrinsics that has a token type argument, which are general intrinsics and not necessarily only for windows, and they are handled within [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L5051 | `SelectionDAGBuilder::visitIntrinsicCall` ]]. For example, [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/include/llvm/IR/Intrinsics.td#L742-L745 | `eh.exceptionpointer` ]] and [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/include/llvm/IR/Intrinsics.td#L747-L748 | `eh.exceptioncode` ]] intrinsics are lowered [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L6209-L6222 | here ]].
We can do similarly, but to do that we have to do custom lowering within `SelectionDAGBuilder::visitIntrinsicCall`, meaning we can't do it in somewhere like WebAssemblyISelLowering.cpp. The reason is, it [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L5060 | calls ]] [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L4305 | `SelectionDAGBuilder::visitTargetIntrinsic` ]] for not-listed intrinsics, within which it [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L4314-L4323 | builds ]] the operand list. Token type operands cannot go through this process. All other intrinsics with token types are processed within `SelectionDAGBuilder::visitIntrinsicCall` and do not enter `SelectionDAGBuilder::visitTargetIntrinsic`.
I didn't want to put too much target-specific routines in SelectionDAGBuilder so I came up with with this extra intrinsic `wasm.extract.exception`. But actually we already have a wasm specific intrinsic, [[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L6363-L6367 | `wasm.landingpad.index` ]], within `SelectionDAGBuilder::visitIntrinsicCall` already, for exactly the same reason (token type argument thing), so maybe one more wouldn't matter too much? ¯\_(ツ)_/¯
================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:314
+ // readability.
+ if (!WasmKeepRegisters)
+ break;
----------------
dschuff wrote:
> Shouldn't this be an assert? i.e. if WasmKeepRegisters is disabled this should have been lowered away, right?
Not sure what you mean. `WasmKeepRegisters` deletes not instructions but register operands, so this is the only place instruction can be omitted in printing.
And even register operands have not been moved by now. The operand-removing
[[ https://github.com/llvm/llvm-project/blob/29ad802db08206000318b7445592abd620ccf650/llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp#L269-L270 | takes places ]] within `WebAssemblyMCInstLower::Lower`, which is called 6 lines below here. So here we still have register operands, whether or not we have `-wasm-keep-registers`.
================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrControl.td:144
let isTerminator = 1, hasCtrlDep = 1, isBarrier = 1 in {
-defm THROW_I32 : I<(outs), (ins event_op:$tag, I32:$val),
- (outs), (ins event_op:$tag),
- [(WebAssemblythrow (WebAssemblywrapper texternalsym:$tag),
- I32:$val)],
- "throw \t$tag, $val", "throw \t$tag",
- 0x08>;
-defm THROW_I64 : I<(outs), (ins event_op:$tag, I64:$val),
- (outs), (ins event_op:$tag),
- [(WebAssemblythrow (WebAssemblywrapper texternalsym:$tag),
- I64:$val)],
- "throw \t$tag, $val", "throw \t$tag",
- 0x08>;
-defm RETHROW : NRI<(outs), (ins bb_op:$dst), [], "rethrow \t$dst", 0x09>;
-let isCodeGenOnly = 1 in
-// This is used when the destination for rethrow is the caller function. This
-// will be converted to a rethrow in CFGStackify.
-defm RETHROW_TO_CALLER : NRI<(outs), (ins), [], "rethrow">;
+defm THROW : I<(outs), (ins event_op:$tag, variable_ops),
+ (outs), (ins event_op:$tag),
----------------
dschuff wrote:
> dumb question: why is throw variadic now?
In the new proposal, `throw` and `br_on_exn` takes a tag argument, and depending on the tag, there can be multiple values to throw or extract. `throw` can pop multiple values from the stack to make an except_ref, and `br_on_exn` can push multiple values onto the stack. For C++ exceptions, we only use a single i32 value whose signature is `(i32)->()`. But by the spec we can have other tags whose signatures involve multiple value types.
(In order to share signatures with functions, all exceptions (= events)' signatures are assumed to have a void return type.)
>From the spec:
```
The throw instruction takes an exception index as an immediate argument. That index is used to identify the exception tag to use to create and throw the corresponding exception.
The values on top of the stack must correspond to the type associated with the exception. These values are popped off the stack and are used (along with the corresponding exception tag) to create the corresponding exception. That exception is then thrown.
```
================
Comment at: lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp:211
+// another tag, but for now we only test for __cpp_exception tag, and if it does
+// not match, i.e., it is a foreign exception, we rethrow it.
+//
----------------
dschuff wrote:
> Side question, do we currently just rethrow it immediately or do we run destructors first?
We rethrow it immediately and we should. If there are any destructors that have to be run, they are gonna run in cleanup pads in the enclosing scope of this scope.
================
Comment at: lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp:355
-// getEHScopeMembership() function will not be called after this, so it is fine.
-bool WebAssemblyLateEHPrepare::mergeTerminatePads(MachineFunction &MF) {
- SmallVector<MachineBasicBlock *, 8> TermPads;
----------------
This was intended for code size optimization, and this is actually something we might still need even with the second proposal. (Of course, this function in the current state wouldn't work, it needs adjusting to the new proposal) I think we'll delete this here now anyway and bring this function again later when multiple terminate pads actually become code size problems.
================
Comment at: lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:321
+ // 'catch' and 'extract_exception' should be the first instruction of a BB and
+ // cannot move.
+ if (Def->getOpcode() == WebAssembly::CATCH ||
----------------
dschuff wrote:
> Is there any danger of any other passes trying to move these instructions around?
Don't think so. The pass order is
```
...
FixIrreducibleControlFlow
LateEHPrepare // These instructions are created here
// Register stackification related transformations
PrepareForLiveIntervals
OptimizeLiveIntervals
MemIntrinsicResults
RegStackify // We are here
RegColoring
ExplicitLocals
CFGSort
CFGStackify
...
```
As you can see, other than RegStackify, no pass reorders instructions. And also there are no pass that modify CFG after LateEHPrepare. (To fix unwinding mismatches for EH (D48345), we have to modify CFG after CFGStackify later, but that's another story)
================
Comment at: test/CodeGen/WebAssembly/cfg-stackify-eh.ll:24
+; CHECK: block except_ref
+; CHECK: br_on_exn 0, __cpp_exception at EVENT, $[[EXCEPT_REF]]
+; CHECK: rethrow
----------------
dschuff wrote:
> Is there any way to get a nice annotation for `br_on_exn` like we have for br/rethrow?
> actually for that matter, I notice that we also don't have basic block labels (e.g. `.LBB0`) in these tests anymore. I assume they are still there but you are just not `CHECK`ing for them?
- Is there any way to get a nice annotation for br_on_exn like we have for br/rethrow?
That should be already printed. I'll add `br_on_exn` in the annotation test too.
- actually for that matter, I notice that we also don't have basic block labels (e.g. .LBB0) in these tests anymore. I assume they are still there but you are just not CHECKing for them?
Yes they are there but I just deleted the checking lines, because they don't look very important and their numbers are subject to be incremented or decremented depending on the context. Do you think it's better to restore them?
================
Comment at: test/MC/WebAssembly/annotations.s:1
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
+
----------------
aheejin wrote:
> dschuff wrote:
> > are simd128 and nontrapping-fptoint needed here?
> I deleted `annotations.mir` and created this instead for two reasons:
> - Given that we cannot serialize WasmFunctionInfo in mir files, it is not possible to make mir files with registers pass the assembler, because the assembler queries if each register is stackified or not in WasmFuncInfo.
> - This is a better way to test the annotation only anyway. I don't remember why I didn't do this in the first place. :$
are simd128 and nontrapping-fptoint needed here? -> No. I'll delete them.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57134/new/
https://reviews.llvm.org/D57134
More information about the llvm-commits
mailing list