[PATCH] D74191: [WebAssembly] Implement multivalue call_indirects

[Thomas Lively via Phabricator via llvm-commits]

tlively 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);
----------------
aheejin wrote:
> 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?
I got this working using `unsigned NumDefs = MO->getParent()->getNumExplicitDefs();`, but I'm not sure this would be a good change to make. The MachineVerifier is supposed to be verifying that each machine instruction matches its definition in its MCID. By changing `NumDefs` to be derived from the machine instruction rather than the MCID, we are changing MachineVerifier so that it instead validates instructions against themselves, which is a much less powerful check.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:77
+// TODO: delete CALL_*
+let variadicOpsAreDefs = 1 in
 defm CALL :
----------------
aheejin wrote:
> So this does not affect the capability to handle variadic use ops in later passes? (e.g. `MachineOperand::isUse()` correctly return true for uses?)
At the Machine IR layer, each operand individually knows whether it is a use or a def, so `MachineOperand::isUse()` and friends still work correctly. At the MC layer everything is horribly broken, but that's ok because we don't have registers there except for tests. This accounts for some extra care I had to take with MCOperands in the final patch in this sequence.


================
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];
----------------
aheejin wrote:
> Can we change this condition to `I < MI->getNumOperands()`?
This doesn't seem to work. I haven't really dug into why, but I could if you'd like.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyUtilities.cpp:138
   case WebAssembly::CALL_INDIRECT_exnref_S:
     return MI.getOperand(1);
   case WebAssembly::CALL:
----------------
aheejin wrote:
> Not about this CL, but this actually looks incorrect, doesn't it? I guess this function was never used for call_indirects after all...
Yes, this is very wrong. I believe I ended up fixing it in a later patch in this sequence.


================
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:
----------------
aheejin wrote:
> What's the difference between `getNumDefs` and `getNumExplicitDefs`? Do we have any implicit defs?
Yes, one example of an implicit def is `ARGUMENTS`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74191