[PATCH] D74191: [WebAssembly] Implement multivalue call_indirects

[Heejin Ahn via Phabricator via llvm-commits]

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

Nice!



================
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);
----------------
tlively wrote:
> 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.
I think you're right.


================
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];
----------------
tlively wrote:
> 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.
Yeah I think that's more general statement that we don't need to subtract `NumVariadicDefs` separately. (And this is not MachineVerifier so we are not loosening checks) Digging into why sounds OK, but I don't think we need to spend too much time there if it does not work.


================
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:
----------------
tlively wrote:
> 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`.
Then I wonder how the previous code for `CALL` worked, because it uses `getNumDefs`..?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74191