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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin accepted this revision.
aheejin added a comment.
This revision is now accepted and ready to land.

Sorry for the delayed reply. I somehow assumed this CL was still in progress and didn't know it was finalized long ago! LGTM.



================
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:
> > 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.
I'd be also OK with leaving this function in this file in the current state, but isn't this function gonna be reduced to ~5 lines after we delete all other `CALL_VOID` and `CALL_type` variants? This function may not be really about opcodes by that point then. How about moving this to WebAssemblyUtilities.cpp/h or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71484