[PATCH] D57134: [WebAssembly] Exception handling: Switch to the new proposal

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:252
+  ExtractExnF =
+      Intrinsic::getDeclaration(&M, Intrinsic::wasm_extract_exception);
+
----------------
aheejin wrote:
> 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? ¯\_(ツ)_/¯
I'd say if putting support for `wasm.get.exception` support in SelectionDAGBuilder means we can get away with one less intrinsic, then it might be worth it.
Would that be a straightforward change to make after this CL lands? e.g. could just put a TODO here and follow up?


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:314
+    // readability.
+    if (!WasmKeepRegisters)
+      break;
----------------
aheejin wrote:
> 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`.
Oh, I didn't realize that you didn't set `isPseudo` on the instruction definition above. I think if you do that it won't be printed by MC at all. In that case I guess it will be hard to write tests for it, right?


================
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
----------------
aheejin wrote:
> 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?
No, it looks good with br_on_exn added.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57134/new/

https://reviews.llvm.org/D57134