[PATCH] D71484: [WebAssembly][InstrEmitter] Foundation for multivalue call lowering

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Mostly LGTM to me



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:837
+  bool HasPhysRegOuts = NumResults > NumDefs &&
+                        II.getImplicitDefs() != nullptr && !HasVRegVariadicDefs;
 #ifndef NDEBUG
----------------
Can't variadic-def call instructions also have implicit defs, such as `$argument`, `$sp32`, and `$sp64`?


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:512
 /// instruction.
-inline unsigned getCalleeOpNo(unsigned Opc) {
-  switch (Opc) {
+inline const MachineOperand &getCalleeOp(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
----------------
I think the intention of D64643 was this file only contains methods that can work on `unsigned Opc` so that they can work equally on `MachineInstr`s and `MCInst`s. I now see that this function is not used for `MCInst`s now, but I think it'd better to make functions in this file consistently take `unsigned Opc`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:219
+      if (Op->getOpcode() == WebAssemblyISD::Wrapper)
+        Op = Op->getOperand(0);
+      Ops.push_back(Op);
----------------
Why take out the wrapper?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:63
+defm CALL :
+  I<(outs), (ins function32_op:$callee, variable_ops),
+    (outs), (ins function32_op:$callee), [],
----------------
Just to clarify, even if `variable_ops` is within `(ins)`, if `variadicOpsAreDefs` is 1, are they considered as outputs? What happens if we put `variable_ops` in `(outs)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71484