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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 4 inline comments as done.
tlively added a subscriber: dmitry.
tlively added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:837
+  bool HasPhysRegOuts = NumResults > NumDefs &&
+                        II.getImplicitDefs() != nullptr && !HasVRegVariadicDefs;
 #ifndef NDEBUG
----------------
aheejin wrote:
> Can't variadic-def call instructions also have implicit defs, such as `$argument`, `$sp32`, and `$sp64`?
I suppose they probably could, but I think it would complicate the logic to support mixed virtual and physical variadic and implicit arguments. I'm not sure it's worth trying to do when we don't have any instructions that would test that code path. Maybe @dmitry has a need for such an instruction?


================
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()) {
----------------
aheejin wrote:
> 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`?
We need the MachineInstr in the new case below because the callee op number depends on the number of defs that come before it. Are you suggesting that we move this function to a different file?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:219
+      if (Op->getOpcode() == WebAssemblyISD::Wrapper)
+        Op = Op->getOperand(0);
+      Ops.push_back(Op);
----------------
aheejin wrote:
> Why take out the wrapper?
The wrapper is there to enable pattern matching and is removed by the normal tablegen patterns as well.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:63
+defm CALL :
+  I<(outs), (ins function32_op:$callee, variable_ops),
+    (outs), (ins function32_op:$callee), [],
----------------
aheejin wrote:
> 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)`?
That's correct. Tablegen errors out if you try to put the `variable_ops` in the `(outs)`. This seems like something that could be cleaned up by someone with sufficient motivation, but that person is not me right now!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71484