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

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:314
+    // readability.
+    if (!WasmKeepRegisters)
+      break;
----------------
aheejin wrote:
> 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.
OK, I seemed to have it in my head that there was some pass that removes all pseudos or expects them all to be gone, but I must be mis-remembering because the comment on `isPseudo` says they are expected to be expanded at the latest by `MCInstLower` time which is basically here.
So the instruction maybe should actually be `isPseudo` because there is no encoding.

I think the reason it seemed funny was I thought there was asymmetry between what we print as text and what we encode in binary. But actually the real asymmetry is whether `WasmKeepRegisters` is true (and it just happens that KeepRegisters only works for text anyway). So yeah, I think this is fine in principle.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57134