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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked 2 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:
> > 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?
That means one less intrinsic but we need to do custom lowering (we cannot use tablegen for this), which is not a lot of work though. Yeah, that can be a simple follow-up CL.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:314
+    // readability.
+    if (!WasmKeepRegisters)
+      break;
----------------
dschuff wrote:
> 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?
I don't think `isPseudo` property in .td files make it not printed by MC. I just tested it myself and it was still printed. And we don't use `isPseudo` for any instruction other than `catchret` and `cleanupret` now, which are pseudo instructions that are deleted during LateEHPrepare.

Actually `isCodeGenOnly` and `isPseudo` are currently being used kind of interchangeably in the whole LLVM codebase. We mostly use `isCodeGenOnly` for all pseudo instructions. There is a [[ https://github.com/llvm/llvm-project/blob/b54927cc48920083c0b19586d04b6dffc9de0aa4/llvm/include/llvm/Target/Target.td#L487-L503 | comment block ]] for intended use case of each of `isPseudo` and `isCodeGenOnly`, but I doubt people use these that way. Anyway I think neither of these are related to MC printing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57134