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

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

This is looking really good!



================
Comment at: llvm/lib/CodeGen/MachineOperand.cpp:1174
+  } else
+    OS << ", opaque ";
   if (getAlign() != getBaseAlign())
----------------
Is there a test that demonstrates this printing? Also, I'm not sure specifically calling out zero-sized operands in the text dump is that useful. Can zero-sized operands still be given an alignment? If so, it might even be good to continue printing the alignment for them.


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:132-134
+    // Setting Comdat ensure only one table is left after linking when multiple
+    // modules define the table.
+    Sym->setComdat(true);
----------------
Is this necessary given that the symbol is set to be undefined? Perhaps it would be better to make it defined here and also set comdat so that the linker doesn't need to do anything special to make sure it exists in the final binary. (Or possibly I'm misunderstanding what these settings mean!) @sbc100 wdyt?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1312
 
+bool WebAssemblyTargetLowering::isExternrefGlobal(SDValue Op) const {
+  const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op);
----------------
It would be good to mark these helper functions `static`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:59-61
+  // AS 2 : is an integral address space for globals of externref values
+  // AS 3 : is an non-integral address space for funcref values
+  // AS 4 : is an integral address spaces for globals of funcref values
----------------
Why are the address spaces for globals integral? It doesn't make sense to do arithmetic on the "address" of a global afaict. Does using getelementptr to index tables in the global address space require the address space to be integral? If so, that might be a good reason to make the address space integral, but it might also be a good reason to put tables and globals in different address spaces instead. Also, IMO it would be better to use just a single address space for all globals; that way we could support globals with other types like i32, i64, etc. without even more address spaces.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:1
+
 // WebAssemblyInstrInfo.td-Describe the WebAssembly Instructions-*- tablegen -*-
----------------
Unintentional whitespace change?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:129
+                        TT.isArch64Bit()
+                            ? (hasReferenceTypes(FS)
+                                   ? "e-m:e-p:64:64-i64:64-n32:64-S128-ni:1:3"
----------------
This check should incorporate the CPU as well using `getSubtargetImpl(CPU, FS)->hasReferenceTypes()`.


================
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
+
----------------
pmatos wrote:
> tlively wrote:
> > 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.
> Returning a `ref.null` or an uninitialized externref would make more sense if the return type would be `%externref`. However, in this case this is an `%extern` value, which really is an opaque type and should never really happen.
Ah, that makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425