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

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:91
   if (Subtarget->hasReferenceTypes()) {
     for (auto T : {MVT::externref, MVT::funcref}) {
       setOperationAction(ISD::LOAD, T, Custom);
----------------
tlively wrote:
> 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.
Makes sense.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:821
 
 bool WebAssemblyTargetLowering::getTgtMemIntrinsic(IntrinsicInfo &Info,
                                                    const CallInst &I,
----------------
tlively wrote:
> I expect that you'll have to do something here as well.
I am confused. Is this comment maybe related to D111227 instead?


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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:48
 
+// FIXME: this is sort of duplicated into WebAssemblyISelLowering...
+static bool isRefType(const Type *Ty) {
----------------
tlively wrote:
> Would be good to fix before landing if possible!
Oh, yes, I forgot this. Thanks for pointing it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111154