[PATCH] D74191: [WebAssembly] Implement multivalue call_indirects

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:1598
       if (MO->isReg()) {
-        if (MO->isDef() && !MCOI.isOptionalDef())
+        if (MO->isDef() && !MCOI.isOptionalDef() && !MCID.variadicOpsAreDefs())
           report("Explicit operand marked as def", MO, MONum);
----------------
Can we change the line above
```
unsigned NumDefs = MCID.getNumDefs();
```
to
```
unsigned NumDefs = MO->getParent()->getNumDefs();
```
?

If this works, we don't need to handle this as an exception case. But because this is in target-independent code that affects all targets, some of which can have whatever weird instructions, so I'm not very sure if that will work. How about trying that and see if it works at least, and don't put too much effort on fixing it in case it does not work?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:77
+// TODO: delete CALL_*
+let variadicOpsAreDefs = 1 in
 defm CALL :
----------------
So this does not affect the capability to handle variadic use ops in later passes? (e.g. `MachineOperand::isUse()` correctly return true for uses?)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:211
   const MCInstrDesc &Desc = MI->getDesc();
+  unsigned VariadicDefs = MI->getNumExplicitDefs() - Desc.getNumDefs();
   for (unsigned I = 0, E = MI->getNumOperands(); I != E; ++I) {
----------------
Nit: How about `NumVariadicDefs` to be a bit more consistent in naming?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:234
     case MachineOperand::MO_Immediate:
-      if (I < Desc.NumOperands) {
-        const MCOperandInfo &Info = Desc.OpInfo[I];
+      if (I - VariadicDefs < Desc.NumOperands) {
+        const MCOperandInfo &Info = Desc.OpInfo[I - VariadicDefs];
----------------
Can we change this condition to `I < MI->getNumOperands()`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp:138
   case WebAssembly::CALL_INDIRECT_exnref_S:
     return MI.getOperand(1);
   case WebAssembly::CALL:
----------------
Not about this CL, but this actually looks incorrect, doesn't it? I guess this function was never used for call_indirects after all...


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp:141
   case WebAssembly::CALL_S:
-    return MI.getOperand(MI.getNumDefs());
+    return MI.getOperand(MI.getNumExplicitDefs());
+  case WebAssembly::CALL_INDIRECT:
----------------
What's the difference between `getNumDefs` and `getNumExplicitDefs`? Do we have any implicit defs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74191