[WebAssembly] multivalue stackify fix

[samparker]

Summary

Suggested fix for bugs: https://github.com/llvm/llvm-project/issues/58297, https://github.com/llvm/llvm-project/issues/55136

Add a check that we're not attempting to move a multivalue instruction past any of it's users.

[sbc100]

llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-bug.ll

5

Did you mean to use the wasi target here? We normally use unknown-unknown for testing generic stuff

[samparker]

llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-bug.ll

5

Okay, fair enough, I just copied it from the github issue... I'll make the change.

[samparker]

Updated target triple.

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

339–340

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?

[tlively]

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. The output is checked in as the multivalue-stackify.ll test. It may be worth playing around with that script so that it generates programs that exhibit this problem.

[samparker]

llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

339–340

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!

[samparker]

Using mir test and updated comment.

[samparker]

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..?

[tlively]

In 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.

llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

339–340

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.

llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-past-use.mir

10 ↗

(On Diff #475070)

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.

[samparker]

llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-past-use.mir

10 ↗

(On Diff #475070)

Yeah, good point. I'll take the reproducer from here since trying to simplify fp128 lowering is too much work!

[samparker]

Using smaller reproducer.

[tlively]

LGTM, thanks!

llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

static bool isSafeToMove(const MachineOperand *Def, const MachineOperand *Use,
   // same instruction as subsequent defs.
   if (Def != DefI->defs().begin())
     return false;
 
   // If any subsequent def is used prior to the current value by the same
   // instruction in which the current value is used, we cannot
   // stackify. Stackifying in this case would require that def moving below the
   // 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())) {
-    for (const auto &PriorUse : UseI->uses()) {
+    auto I = std::next(MachineBasicBlock::const_iterator(DefI));
+    auto E = std::next(MachineBasicBlock::const_iterator(UseI));
+    for (; I != E; ++I) {
+      for (const auto &PriorUse : I->uses()) {
         if (&PriorUse == Use)
           break;
         if (PriorUse.isReg() && SubsequentDef.getReg() == PriorUse.getReg())
           return false;
       }
     }
+  }
 
   // If moving is a semantic nop, it is always allowed
   const MachineBasicBlock *MBB = DefI->getParent();
   auto NextI = std::next(MachineBasicBlock::const_iterator(DefI));
   for (auto E = MBB->end(); NextI != E && NextI->isDebugInstr(); ++NextI)
     ;
   if (NextI == Insert)
     return true;

llvm/test/CodeGen/WebAssembly/multivalue-dont-move-def-bug.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -verify-machineinstrs -mattr=+multivalue | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+@d = hidden local_unnamed_addr global double 0.000000e+00, align 8
+@c = hidden local_unnamed_addr global i32 0, align 4
+
+define hidden fp128 @e() local_unnamed_addr #0 {
+; CHECK-LABEL: e:
+; CHECK:         .functype e () -> (i64, i64)
+; CHECK-NEXT:    .local i64, i64, f64, i32, i32
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    block
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    f64.load d
+; CHECK-NEXT:    local.tee 2
+; CHECK-NEXT:    f64.const 0x0p0
+; CHECK-NEXT:    f64.ne
+; CHECK-NEXT:    br_if 0 # 0: down to label0
+; CHECK-NEXT:  # %bb.1:
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    i32.load c
+; CHECK-NEXT:    local.set 3
+; CHECK-NEXT:    block
+; CHECK-NEXT:    block
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    f64.abs
+; CHECK-NEXT:    f64.const 0x1p31
+; CHECK-NEXT:    f64.lt
+; CHECK-NEXT:    i32.eqz
+; CHECK-NEXT:    br_if 0 # 0: down to label2
+; CHECK-NEXT:  # %bb.2:
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.trunc_f64_s
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:    br 1 # 1: down to label1
+; CHECK-NEXT:  .LBB0_3:
+; CHECK-NEXT:    end_block # label2:
+; CHECK-NEXT:    i32.const -2147483648
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:  .LBB0_4:
+; CHECK-NEXT:    end_block # label1:
+; CHECK-NEXT:    local.get 4
+; CHECK-NEXT:    call __floatsitf
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    i64.select
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    i64.const 0
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    i64.select
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:  .LBB0_5:
+; CHECK-NEXT:    end_block # label0:
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    # fallthrough-return
+  %1 = load double, ptr @d, align 8
+  %2 = fcmp une double %1, 0.000000e+00
+  br i1 %2, label %9, label %3
+
+3:                                                ; preds = %0
+  %4 = load i32, ptr @c, align 4
+  %5 = icmp eq i32 %4, 0
+  %6 = fptosi double %1 to i32
+  %7 = sitofp i32 %6 to fp128
+  %8 = select i1 %5, fp128 0xL00000000000000000000000000000000, fp128 %7
+  br label %9
+
+9:                                                ; preds = %0, %3
+  %10 = phi fp128 [ %8, %3 ], [ 0xL00000000000000000000000000000000, %0 ]
+  ret fp128 %10
+}