[PATCH] D71496: [WebAssembly] Split and recombine multivalue calls for ISel
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 16 10:34:49 PST 2019
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
More information about the llvm-commits
mailing list