[PATCH] D137824: [WebAssembly] multivalue stackify fix

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

In D137824#3924406 <https://reviews.llvm.org/D137824#3924406>, @samparker wrote:

>> I don't know if you've seen it, but we have a multivalue-stackify.py script that tries to exhaustively enumerate multivalue sequences up to a certain size then filter out the uninteresting ones.
>
> Cool! My python isn't great tbh... but looking through the generated test, I would have thought that `f141` may have triggered this bug:
>
>   %t0 = call {i32, i32} @op_0_to_2()
>   %t1 = extractvalue {i32, i32} %t0, 1
>   call void @op_1_to_0(i32 %t1)
>   %t2 = extractvalue {i32, i32} %t0, 0
>   call void @op_1_to_0(i32 %t2)
>   ret void
>
>   %0:i32, %1:i32 = CALL @op_0_to_2, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
>   CALL @op_1_to_0, %1, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
>   CALL @op_1_to_0, %0, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64
>   RETURN implicit-def dead $arguments
>
> Is it the fact that the users are calls and this prevents a move from being attempted..?

Yeah, I suspect you're right that the function calls are inhibiting the moves. It would be nice to find a way around that, but looking at the code it looks like it will refuse to reorder a pair of calls no matter what attributes the callee has. I suppose we could add a debug-only flag to change that behavior, but it's probably not worth it. Maybe the script could emit instructions other than function calls instead...

Anyway, mostly LGTM with the exception that it would be good to make the test even easier to read.



================
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())) {
----------------
samparker wrote:
> 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!
> 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?

Ah, you're right. I had forgotten that we're moving defs here.


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-past-use.mir:10
+
+  define hidden fp128 @e() local_unnamed_addr #0 {
+    %1 = load double, ptr @d, align 8
----------------
Is it possible to come up with a reproducer with a single basic block? Either way, it would be helpful to add comments highlighting the important pieces of the test. There's enough going on here that it's difficult to figure out which parts are important just by looking at it.


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

https://reviews.llvm.org/D137824