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

[Andy Wingo via Phabricator via llvm-commits]

wingo added a comment.

Really neat patch, glad to finally see codegen for externref!!!  Comments are a bunch of nits / questions, really -- I am not an LLVM expert so please take them with a salt lick.



================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:215
     /// Return true if the bit size is a multiple of 8.
     bool isByteSized() const { return getSizeInBits().isKnownMultipleOf(8); }
 
----------------
Should this return false when isZeroSized() ?


================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1170
   MachineOperand::printOperandOffset(OS, getOffset());
-  if (getAlign() != getSize())
-    OS << ", align " << getAlign().value();
+  if (getSize() > 0) {
+    if (getAlign() != getSize())
----------------
Perhaps add isZeroSized predicate here, for readability.

Generally speaking, it might be nice to add `assert(!isZeroSized())` to the implementation of all relevant `getSize()` methods, just to make sure you (and future developers) are alerted when they fail to explicitly handle the externref case.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4105
+
+    if (EltVT.getSizeInBits() == 0)
+      A = Ptr;
----------------
`isZeroSized`.  or `!hasSize` ?  Dunno.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4118
 
-    if (MemVTs[i] != ValueVTs[i])
+    // Skip ZExt or Trunc if a ValueVT is zero sized
+    if (ValueVTs[i].getSizeInBits() > 0 && MemVTs[i] != ValueVTs[i])
----------------
Comments like this make me think there is a property that we're currently calling "zero sized" but which probably has a better name.  Anyway just a thought.


================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:35
+      Wasm_FuncrefTy(C, Type::Wasm_FuncrefTyID), Int1Ty(C, 1), Int8Ty(C, 8),
+      Int16Ty(C, 16), Int32Ty(C, 32), Int64Ty(C, 64), Int128Ty(C, 128) {}
 
----------------
Probably something we don't want clang-format to do?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1302
+  const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op);
+  if (!GA || GA->getAddressSpace() != 2)
+    return false;
----------------
Symbolic names for address spaces needed here and below


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1330
+
+  // Expect Offset to be undef for externref stores
+  const SDValue &Offset = SN->getOffset();
----------------
What is an externref store?  A store of an externref value?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2148
+    unsigned NumParts, MVT PartVT, Optional<CallingConv::ID> CC) const {
+  if (PartVT.getSizeInBits() == 0) {
+    Parts[0] = Val;
----------------
What is this?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2159
+  if (ArgTy->isWasm_ReferenceTypeTy())
+    return Align(4);
+  
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:48
 
+  MVT getPointerTy(const DataLayout &DL, uint32_t AS = 0) const override {
+    if (AS == 1)
----------------
Need a big comment somewhere describing the relationship between address spaces and reftypes.  Use symbolic names instead of 1 and 3 ?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp:110
     Sym->setFunctionTable();
     // The default function table is synthesized by the linker.
     Sym->setUndefined();
----------------
This would not be the case for the call_ref table, right?  Rather it would have fixed size 1 and be defined as comdat.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-globalget.ll:4
+%extern = type opaque
+%externref = type %extern addrspace(1)* ;; addrspace 1 is nonintegral
+
----------------
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`.


================
Comment at: llvm/test/CodeGen/WebAssembly/funcref-call.ll:17
+; CHECK-NEXT: local.get 0
+; CHECK-NEXT: call_indirect __funcref_call_table, () -> ()
+; CHECK-NEXT: end_function
----------------
We should clear the table entry after the call, to prevent hidden GC roots.  It's overhead now but I would make it unconditional, in anticipation of `call_ref`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425