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

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:69
     addRegisterClass(MVT::v2f64, &WebAssembly::V128RegClass);
+  } else if (Subtarget->hasReferenceTypes()) {
+    addRegisterClass(MVT::externref, &WebAssembly::EXTERNREFRegClass);
----------------
should be just "if"


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1086
+    SDValue TableSet = DAG.getMemIntrinsicNode(
+        WebAssemblyISD::TABLE_SET_FUNCREF, DL, DAG.getVTList(MVT::Other),
+        TableSetOps, MVT::funcref,
----------------
Should this be TABLE_SET_FUNCREF and not just TABLE_SET ?  Given that we're producing a DAG node that then needs to be selected, I would think the type-specific stuff could happen in the pattern-matching.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1334
+
+  SDVTList Tys = DAG.getVTList(VT);
+  SDValue Ops[] = {SN->getChain(), Value, Base};
----------------
wingo wrote:
> Given that this node defines one value, the chain, should this be MVT::other?
Pretty sure this comment still applies


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425