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

[Paulo Matos via Phabricator via llvm-commits]

pmatos marked 2 inline comments as done.
pmatos added inline comments.


================
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); }
 
----------------
wingo wrote:
> Should this return false when isZeroSized() ?
I agree that this should probably return false in that case.


================
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())
----------------
wingo wrote:
> 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.
Cannot really use `isZeroSized` here because this is for `MachineMemOperands` - we defined `isZeroSized` for VTs.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4105
+
+    if (EltVT.getSizeInBits() == 0)
+      A = Ptr;
----------------
wingo wrote:
> `isZeroSized`.  or `!hasSize` ?  Dunno.
Sounds like I should have used `isZeroSized` here.


================
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])
----------------
wingo wrote:
> 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.
but isn't the property really if it's zero sized? we don't want to mention any target dependent property here like reference type, so this feels like the most generic name. 


================
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) {}
 
----------------
wingo wrote:
> Probably something we don't want clang-format to do?
right - wonder how to stop clang-format to not do this.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1332-1333
+  const SDValue &Offset = SN->getOffset();
+  if (!Offset->isUndef())
+    return Op;
+
----------------
tlively wrote:
> 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?
This is for globals, so Offset is undef. I guess once we do tables, we'll need to enable other offsets here.


================
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;
----------------
wingo wrote:
> What is this?
At some point in an load/store optimization this is called and the default implementation crashes on opaque types, so we need to override this. Although we could use here `isZeroSized()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425