[PATCH] D95425: Implementation of global.get/set for reftypes in LLVM IR

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1332-1333
+  const SDValue &Offset = SN->getOffset();
+  if (!Offset->isUndef())
+    return Op;
+
----------------
This seems a little strange. Why not allow 0? Once we support user-defined tables and not just individual globals, will this need to allow any non-negative offset?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2159
+  if (ArgTy->isWasm_ReferenceTypeTy())
+    return Align(4);
+  
----------------
wingo wrote:
> What does alignment mean in the context of reference types?  We can't spill them to the stack.  Are you just specifying a least common denominator here?  Comment needed in any case.
I would also expect `Align(1)` to make more sense here, since table slot indices do not need to be divisible by any particular alignment.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:86-92
+// Custom Funcref call node
+// This is lowered to table.set+call_indirect with reference types
+// But if we have in addition typed function references we can use call_ref
+def wasm_call_ref_t : SDTypeProfile<0, -1, [SDTCisVT<0, funcref>]>;
+def wasm_call_ref : SDNode<"WebAssemblyISD::CALL_REF", wasm_call_ref_t,
+                           [SDNPHasChain, SDNPOutGlue, SDNPOptInGlue,
+                            SDNPVariadic]>;
----------------
Is this used anywhere?


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-globalget.ll:4
+%extern = type opaque
+%externref = type %extern addrspace(1)* ;; addrspace 1 is nonintegral
+
----------------
wingo wrote:
> A design document for how this works would really be helpful :)  So LLVM is going to consider that any pointer to address space 1 has MVT exterrnref at lowering-time?  It's interesting to go down this route rather than adding a new `llvm::Type`.
We essentially consider changes to LLVM IR to be out of scope to the extent possible. If we only touch code in the WebAssembly backend and maybe fix a few bugs in target independent code, then we are comfortable signing off on patches within the team, but if we do anything non-trivial in target independent code, we need sign off from the wider LLVM community. Changes to LLVM IR would require a full-blown RFC process and I expect they would not be accepted.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-undef.ll:13-14
+; CHECK-LABEL: return_extern_undef:
+; CHECK-NEXT: functype       return_extern_undef () -> ()
+; CHECK-NEXT: end_function
+
----------------
I would expect this to declare an externref local and return it uninitialized or to return a ref.null. It would be good to at least add a TODO comment for fixing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425