[PATCH] D71496: [WebAssembly] Split and recombine multivalue calls for ISel

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:224
+    MachineSDNode *CallParams =
+        CurDAG->getMachineNode(WebAssembly::CALL_PARAMS, DL, MVT::i32, Ops);
+    SDValue Link(CallParams, 0);
----------------
aheejin wrote:
> Nit: Does this `i32` have a meaning? If not, can we use a chain (`MVT::Other`) instead?
There's no meaning, but I have not been able avoid assertion failures using `MTV::Other`. I'll investigate further without spending too much time on it.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:433
+                                           const TargetInstrInfo &TII) {
+  MachineInstr &CallParams = *CallResults.getPrevNode();
+  assert(CallParams.getOpcode() == WebAssembly::CALL_PARAMS);
----------------
aheejin wrote:
> Is `CallParams` guaranteed to be the previous node? What we did in ISelDAGToDAG is to make `CallParams` an operand of `CallResult`. While they have data dependences and `CallResult` will be guaranteed to be selected after `CallParams`, I'm not sure if there can't be any other instructions in between. Can't we get the operand of `CallResults`?
Yeah, good point. I didn't see a simple API for getting defs of operands from MachineInstrs, but making this more robust is probably worth some extra code.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:444
+  for (auto Use : CallParams.uses())
+    MIB.add(Use);
+
----------------
aheejin wrote:
> How do we assign which is defs and which is uses in this new `CALL` instruction?
MachineOperands store this information on themselves and we follow the convention here of having the defs before the uses so things like `getNumDefs` should still work. This will be a bigger problem at the MC layer, where the def and use information is erased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71496