[PATCH] D123484: [WebAssembly] Implement ref.is_null MC layer support and codegen

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h:83
 
+inline bool valTypeIsRefType(wasm::ValType Type) {
+  return Type == wasm::ValType::EXTERNREF || Type == wasm::ValType::FUNCREF;
----------------
Can this be just `isRefType`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrRef.td:30-32
+  // Define register variants of ref.is_null for each reference type. A
+  // single stack variant of the instruction is defined later.
+  let isCodeGenOnly = 1 in
----------------
asb wrote:
> aheejin wrote:
> > Why should the stack version and the non-stack version separately? Doesn't `I` provide a template to define both at the same time? Maybe I'm not familiar with the context.
> > 
> > Also why is this `isCodeGenOnly`?
> It does, but due to the way representation of reftypes is currently handled we want two register versions of the instruction (one for externref and one for funcref), but only one for the stack-based version (so `ref.is_null` can be printed/parsed in assembly). Marking the register instructions as isCodeGenOnly is consistent with what the `I` multiclass does (and correct, as the register instructions are really pseudo-instructions to bridge the gap between LLVM being register based on wasm being stack-based).
> 
> As I note in the patch summary I don't think this necessarily the globally optimum way of handling reftypes, but I want to save doing more invasive changes / refactoring until we've properly planned out handling of reftypes looking ahead to the typed function ref and GC proposals.
Why should there be only one stack variant? I think two different instructions can have the same name and opcode so they can be parsed and emitted fine?


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

https://reviews.llvm.org/D123484