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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice! Are there no tests because we don't have the assembly printer support yet?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:224
+    MachineSDNode *CallParams =
+        CurDAG->getMachineNode(WebAssembly::CALL_PARAMS, DL, MVT::i32, Ops);
+    SDValue Link(CallParams, 0);
----------------
Nit: Does this `i32` have a meaning? If not, can we use a chain (`MVT::Other`) instead?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:433
+                                           const TargetInstrInfo &TII) {
+  MachineInstr &CallParams = *CallResults.getPrevNode();
+  assert(CallParams.getOpcode() == WebAssembly::CALL_PARAMS);
----------------
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`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:444
+  for (auto Use : CallParams.uses())
+    MIB.add(Use);
+
----------------
How do we assign which is defs and which is uses in this new `CALL` instruction?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:59
+// results, but this is not possible to model directly. Instead, we
+// select calls to a CALL taking variadic aguments linked with a
+// CALL_RESULTS that handles producing the call's variadic results. We
----------------
`CALL`->`CALL_PARAMS`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:63
+// passes over MachineInstrs will only ever observe CALL nodes with
+// all of the expected variadic uses and defs.
+let isPseudo = 1 in
----------------
Sorry for real nit: Can we make comments wrap to 80 cols..? Lines kind of look to short.. I think there should be a shortcut in most editors for doing that.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:70
+
+let variadicOpsAreDefs = 1, usesCustomInserter = 1 in
+defm CALL_RESULTS :
----------------
Isn't this also `isPseudo = 1`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71496