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

[Andy Wingo via Phabricator via cfe-commits]

wingo added inline comments.


================
Comment at: clang/lib/Basic/Targets/WebAssembly.h:150
       : WebAssemblyTargetInfo(T, Opts) {
-    resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128");
+    resetDataLayout("e-m:e-p:32:32-i64:64-n32:64-S128-ni:1");
   }
----------------
This change is already in D101608; rebase?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4083
   EVT PtrVT = Ptr.getValueType();
+  EVT EltVT = PtrVT.getScalarType();
 
----------------
It turns out that all the changes here to `visitLoad` and `visitStore` are no longer needed.  The issue was that before we made `getPointerMemTy` virtual, it was hard-coded to return an integer of whatever the pointer bit size was.  But for externref and funcref values, where we're using pointers in non-default address spaces to represent opaque values, that's not what we want: an externref "in memory" isn't an i32.  Having made `getPointerMemTy` virtual and returning externref / funcref for those values, now we avoid the zero-extension paths, and even the ISD::ADD node doesn't cause us problems.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4257
+    // Skip ZExt or Trunc if a ValueVT is zero sized
+    if (!ValueVTs[i].isZeroSized() && MemVTs[i] != ValueVTs[i])
       Val = DAG.getPtrExtOrTrunc(Val, dl, MemVTs[i]);
----------------
Can revert this too, as mentioned above.


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:1684
   Type *Ty = VT.getTypeForEVT(Context);
-  if (Alignment >= DL.getABITypeAlign(Ty)) {
+  if (!VT.isZeroSized() && Alignment >= DL.getABITypeAlign(Ty)) {
     // Assume that an access that meets the ABI-specified alignment is fast.
----------------
This one I would think that we want to return true, that the access is "fast", for access to zero-sized types.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:2347
+  return false;
+}
----------------
I just checked and it would seem that the `getPointerMemTy` change means that this override no longer appears to be needed.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:15
 multiclass TABLE<WebAssemblyRegClass rt> {
+  let mayLoad = 1 in
   defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
----------------
I think you may need `hasSideEffects = 0` for these annotations to have an effect.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:19
                          [],
                          "table.get\t$res, $table",
                          "table.get\t$table",
----------------
Not something for this patch, but this is certainly bogus: surely we mean `table.get\t$table, $i` and we have an i32 index argument?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:59
+          (TABLE_SET_EXTERNREF i32:$table, i32:$idx, externref:$r)>,
+          Requires<[HasReferenceTypes]>;
+
----------------
Consider changing to use the approach used for `GLOBAL_GET` from the parent commit, and folding these into the multiclass so that we don't have to repeat the code for externref and funcref.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425