[PATCH] D137824: [WebAssembly] multivalue stackify fix

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:339-340
   // current def in the stack, which cannot be achieved, even with locals.
+  // If any subsequent def is used prior by a different instruction, we also
+  // cannot stackify.
   for (const auto &SubsequentDef : drop_begin(DefI->defs())) {
----------------
tlively wrote:
> This doesn't sound quite right. As a counterexample, consider
> 
> ```
> a, b = foo
> bar a
> baz b
> ```
> 
> Here we have a subsequent def (of `b`) used by a different instruction (`baz`), but we can still stackify by moving `baz` before `bar`.
> 
> It would be good to reduce the test case to figure out more precisely what the problem is and come up with a more precise solution. Maybe the problem is that `baz` and `bar` (or rather their equivalents in the test) cannot be reordered?
Maybe my comment doesn't make complete sense... But just to clarify, we move the def, not the user, right? And we only attempt to move considering the first def?

So I think your example is the pattern that the existing code will handle just fine: we won't attempt to move anything?

IIUC, the case that currently isn't handled is this:


```
a, b = foo
baz b
bar a
```

The existing code only looks at `bar` and will attempt to sink `foo` past `baz`, producing broken code. I'll convert to the test to a MIR which should demonstrate that this is precisely what's going on.

If I'm misunderstanding, please shout!


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

https://reviews.llvm.org/D137824