[PATCH] D89797: [WebAssembly] Implementation of (most) table instructions

[Paulo Matos via Phabricator via llvm-commits]

pmatos added a comment.

Is this good to merge?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:316
+defm "" : LOCAL<FUNCREF>, Requires<[HasReferenceTypes]>;
+defm "" : LOCAL<EXTERNREF>, Requires<[HasReferenceTypes]>;
 
----------------
tlively wrote:
> pmatos wrote:
> > sbc100 wrote:
> > > How is that we were able to get away without these types of changes when implementing the global.get and global.set of externref symbols in `llvm/test/MC/WebAssembly/externref.s`?
> > > 
> > > There is have instructions that take `externref` such as `global.set my_global`.
> > > 
> > > 
> > Yes - at first glance, that shouldn't work. But it does work without the changes in this patch so I am stumped and might have to do some looking up to understand it. Maybe @tlively knows more?
> Ah, this is interesting! Remember that llvm-mc only ever uses the stack version of instructions at the MC layer, so in that test the global.get and global.set instructions never actually have any operands of any register class. We also never use global.get or global.set of reference types in any of our instruction selection, so this never came up.
> 
> At a deeper level, this worked because register classes in the WebAssembly backend are fundamentally nothing more than a debugging tool. Since we don't do register allocations and because all register values are erased before we emit modules, everything would continue working correctly if just had a single register class covering all value types. The benefit of having separate register classes is that it enables the MachineInstrVerifier to catch type errors for us. If we actually had a wasm validator in the backend, the separate register classes would not be useful for anything except documentation.
That makes sense. Thanks for the thorough explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89797