[PATCH] D74191: [WebAssembly] Implement multivalue call_indirects

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
tlively added inline comments.


================
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:
> 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.
I'll add a new variable `unsigned DescIndex = I - NumVariadicDefs;` so we at least don't repeat that subexpression.


================
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:
> 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`..?
It looks like this code is only used in analyzing call side effects in WebAssemblyRegStackify. If the retrieved callee op is not a global, the code conservatively assumes it has all kinds of side effects. So we were previously emitting correct but overly-conservative stackifications.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74191