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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

This mostly lgtm.   I would love it if we could avoid adding the new MVT value for now since then we could avoid changing the shared code on llvm/lib/CodeGen/ValueTypes.cpp.. but perhaps that absolutely needed for this change?

lgtm if @tlively is OK with the `.td` changes.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:316
+defm "" : LOCAL<FUNCREF>, Requires<[HasReferenceTypes]>;
+defm "" : LOCAL<EXTERNREF>, Requires<[HasReferenceTypes]>;
 
----------------
pmatos wrote:
> 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.
So can we get away without adding these for now? 


================
Comment at: llvm/utils/TableGen/CodeGenTarget.cpp:232
+  case MVT::externref:
+    return "MVT::externref";
   default: llvm_unreachable("ILLEGAL VALUE TYPE!");
----------------
Maybe use the local style of putting these on single line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89797