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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
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:
> tlively wrote:
> > 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?
> Oh what I meant was, every wasm instruction [[ https://github.com/llvm/llvm-project/blob/0a1e349a7933f7880971533175e11b4bfd22bd53/llvm/lib/Target/WebAssembly/WebAssemblyInstrFormats.td#L37 | implicitly defines ]] `ARGUMENT` register, which is treated as a physical register. Turns out `SP32` and `SP64` are implicitly defined by only [[ https://github.com/llvm/llvm-project/blob/204dfabfe68a978620258baaa0bacb55cbd6859d/llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td#L19 | these two instructions ]], so I think this should be irrelevant.
So yes, nodes can implicitly define physical registers, but that is not sufficient to make `NumResults > NumDefs`, since `NumResults` is the number of explicit outputs of the node. `HasPhysRegOuts` is only `true` when there are explicit ouputs of the node that need to be placed in physical registers. That's why it is correct to set it to `false` when `HasVRegVariadicDefs` is `true`. The implicit physical outputs are not affected by any of this.


================
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:
> tlively wrote:
> > 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?
> Either that, or leave this function here and writing a couple more line in the user side..?
I would be ok moving this function to a different file, but I'm not sure which file that would be. This file contains all the helper functions for WebAssembly ops as far as I can tell, so I would also be ok leaving it here. It makes sense to generalize over MachineInstr and MCInst where possible, but that's just not possible in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71484