[PATCH] D48183: [WebAssembly] Modified tablegen defs to have 2 parallel instuction sets.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added a comment.

Made final comment fixes.. feel free to land :)



================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrFormats.td:39
+// Typically the only difference is that in the stack one the registers are
+// omitted and instead implicit.
+// Every instruction should want to be based on this multi-class to guarantee
----------------
dschuff wrote:
> dschuff wrote:
> > I think the "instead implicit" wording is a bit confusing given that we refer to the stack version as the "explicit locals" version. In this comment I might put even more detail like:
> > "The register versions have virtual-register operands which correspond to wasm locals. Each use and def of the register corresponds to an implicit get_local or set_local operation in wasm. These instructions are used for ISel and all MI passes. The stack versions of the instructions do not have register operands (they implicitly operate on the stack), and get_locals and set_locals are explicit. The register instructions are converted to their corresponding stack instructions before lowering to MC".
> After this lands, it probably makes sense to just turn the explicit-locals pass into the full register->stack conversion pass.
"The register versions have virtual-register operands which correspond to wasm locals": now that I find confusing, since plenty of uses of registers do not correspond to locals.

But you have a point that distinction needs to be made more clear. I'll adapt your text.



================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrFormats.td:39
+// Typically the only difference is that in the stack one the registers are
+// omitted and instead implicit.
+// Every instruction should want to be based on this multi-class to guarantee
----------------
aardappel wrote:
> dschuff wrote:
> > dschuff wrote:
> > > I think the "instead implicit" wording is a bit confusing given that we refer to the stack version as the "explicit locals" version. In this comment I might put even more detail like:
> > > "The register versions have virtual-register operands which correspond to wasm locals. Each use and def of the register corresponds to an implicit get_local or set_local operation in wasm. These instructions are used for ISel and all MI passes. The stack versions of the instructions do not have register operands (they implicitly operate on the stack), and get_locals and set_locals are explicit. The register instructions are converted to their corresponding stack instructions before lowering to MC".
> > After this lands, it probably makes sense to just turn the explicit-locals pass into the full register->stack conversion pass.
> "The register versions have virtual-register operands which correspond to wasm locals": now that I find confusing, since plenty of uses of registers do not correspond to locals.
> 
> But you have a point that distinction needs to be made more clear. I'll adapt your text.
> 
I was going to do it just before "WebAssembly Assembly Printer". There are a whole bunch of passes after Explicit Locals below, and I am not sure how easy it is to make them all deal with stack based instructions:

      WebAssembly Explicit Locals
      WebAssembly CFG Sort
      WebAssembly CFG Stackify
      WebAssembly Lower br_unless
      WebAssembly Register Numbering
      Insert fentry calls
      Insert XRay ops
      Lazy Machine Block Frequency Analysis
      Machine Optimization Remark Emitter
      WebAssembly Assembly Printer



Repository:
  rL LLVM

https://reviews.llvm.org/D48183