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

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:312
+  } else if (Name == "ref.is_null") {
+    // FIXME: overly permissive - accepts any value.
+    if (popType(ErrorLoc, {}))
----------------
tlively wrote:
> How complicated would this be to fix? Is it worth fixing now?
I'd initially skipped it due to planning to return to representation of reftypes in a bigger way. But you're right, it's not hard to provide a better message right now and I've done so by introducing a `popRefType` helper.


================
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
----------------
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/ref-null.ll:45
+  %is_null = call i32 @llvm.wasm.ref.is_null.func(%funcref %null)
+  %arg_is_null = call i32 @llvm.wasm.ref.is_null.func(%funcref %fref)
+  %res = add i32 %is_null, %arg_is_null
----------------
sbc100 wrote:
> Is there some reason why there are no expectations in the body here? (and above?
That's an embarrassing oversight - thanks for catching!

I've updated this test case to use update_llc_test_checks.py (which it seems isn't compatible with -asm-verbose=false).


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

https://reviews.llvm.org/D123484