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

[Paulo Matos via Phabricator via llvm-commits]

pmatos marked 3 inline comments as done.
pmatos added inline comments.


================
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,
----------------
wingo wrote:
> 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.
In this case, I think we cannot generate a `TABLE_SET`. I think the `TABLE_SET` you are referring to is the machine instruction defined in the backend which is not matched from here. From here, you can only match the nodes you create and that's `WebAssemblyISD::TABLE_SET_FUNCREF`. I am still not totally clear why this is, but I never had any luck generating the machine instructions defined in the backend directly from lowering so there might be a way but it's not straightforward. So we generate here the custom machine node we created and then we pattern match that in tablegen to generate the instruction. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425