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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:837
+  bool HasPhysRegOuts = NumResults > NumDefs &&
+                        II.getImplicitDefs() != nullptr && !HasVRegVariadicDefs;
 #ifndef NDEBUG
----------------
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.


================
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()) {
----------------
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..?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71484