[PATCH] D111154: [WebAssembly] Implementation of table.get/set for reftypes in LLVM IR

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

It's very cool to see this all coming together!



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:91
   if (Subtarget->hasReferenceTypes()) {
     for (auto T : {MVT::externref, MVT::funcref}) {
       setOperationAction(ISD::LOAD, T, Custom);
----------------
I think the `MVT::Other` can be rolled into the existing loop. It would also be good to add a comment here about why `MVT::Other` comes up here.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:821
 
 bool WebAssemblyTargetLowering::getTgtMemIntrinsic(IntrinsicInfo &Info,
                                                    const CallInst &I,
----------------
I expect that you'll have to do something here as well.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1429
+  if (IsWebAssemblyGlobal(Op)) {
+    const GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(Op);
+    const GlobalValue *Value = GA->getGlobal();
----------------
This can just be a cast if we can assume that it will succeed. (Or maybe we should be checking for success here?)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1471
 
+  if (IsWebAssemblyTable(Base)) {
+    if (!Offset->isUndef())
----------------
It would be helpful to add a comment here about what DAG patterns we are expecting to find.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1476-1478
+    if (Base->getOpcode() != ISD::ADD || Base->getNumOperands() != 2)
+      report_fatal_error("unexpected base when storing from webassembly table",
+                         false);
----------------
Is there still an ADD when the index is a constant zero?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1487
+
+    const SDNode *PtrOffset = dyn_cast<SDNode>(Base->getOperand(1));
+    if (!PtrOffset || PtrOffset->getOpcode() != ISD::SHL)
----------------
It seems strange to use a dyn_cast for this (and I'm surprised that works at all!)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1551-1575
+    if (Base->getOpcode() != ISD::ADD || Base->getNumOperands() != 2)
+      report_fatal_error("unexpected base when loading from webassembly table",
+                         false);
+
+    GlobalAddressSDNode *GA =
+        dyn_cast<GlobalAddressSDNode>(Base->getOperand(0));
+    if (!GA)
----------------
It would be good to deduplicate this code with the store case as much as possible.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:48
 
+// FIXME: this is sort of duplicated into WebAssemblyISelLowering...
+static bool isRefType(const Type *Ty) {
----------------
Would be good to fix before landing if possible!


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-tableget.ll:8
+
+define %externref @get_externref_from_table(i32 %i) {
+  %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %i
----------------
It would be good to add tests with different access patterns as well like constant indices, base + offset indices, etc.


================
Comment at: llvm/test/CodeGen/WebAssembly/externref-tableget.ll:20
+
+; CHECK: .globl externref_table
----------------
Does this need more information to say it is a table?


================
Comment at: llvm/test/CodeGen/WebAssembly/funcref-table_call.ll:25-27
+; CHECK-NEXT: i32.const       0
+; CHECK-NEXT: ref.null        func
+; CHECK-NEXT: table.set       __funcref_call_table
----------------
Do you think it would be a good idea to skip setting this table slot back to null? I know we discussed this before, but I don't remember what the pros and cons were.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111154