[wasm][liftoff] Fix spill offsets in merge regions

Recompute the spill offsets for values in the merge region, instead of
reusing the offsets of the source. This ensures that spill slots stay
contiguous (modulo alignment).
This also solves a correctness issue where the spill offsets in the
merge region could move up, thereby overwriting the source of another
move.
With this change, the spill offsets always move down (to fill the gap)
or stay the same, such that processing them from bottom to top
can only overwrite sources of already-processed moves.

Since we do not reuse the current state's offsets, this might generate
extra stack moves and regress generated code performance a bit.

Drive-by: print spill offsets in the Liftoff trace

R=clemensb@chromium.org

Bug: v8:12270
Change-Id: I8d20df8fc1e80dd36b6f651de457686e9935a628
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3245115
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77556}

src/wasm/baseline/liftoff-assembler.cc

@@ -481,6 +481,14 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
   InitMergeRegion(this, source_begin + stack_base + discarded,
                   target_begin + stack_base, arity, keep_merge_stack_slots,
                   kConstantsNotAllowed, kNoReuseRegisters, used_regs);
+  // Shift spill offsets down to keep slots contiguous.
+  int offset = stack_base == 0 ? StaticStackFrameSize()
+                               : source.stack_state[stack_base - 1].offset();
+  auto merge_region = base::VectorOf(target_begin + stack_base, arity);
+  for (VarState& var : merge_region) {
+    offset = LiftoffAssembler::NextSpillOffset(var.kind(), offset);
+    var.set_offset(offset);
+  }
 
   // Initialize the locals region. Here, stack slots stay stack slots (because
   // they do not move). Try to keep register in registers, but avoid duplicates.
@@ -708,6 +716,22 @@ void LiftoffAssembler::MaterializeMergedConstants(uint32_t arity) {
   }
 }
 
+#ifdef DEBUG
+namespace {
+bool SlotInterference(const VarState& a, const VarState& b) {
+  return a.is_stack() && b.is_stack() &&
+         b.offset() > a.offset() - element_size_bytes(a.kind()) &&
+         b.offset() - element_size_bytes(b.kind()) < a.offset();
+}
+
+bool SlotInterference(const VarState& a, base::Vector<const VarState> v) {
+  return std::any_of(v.begin(), v.end(), [&a](const VarState& b) {
+    return SlotInterference(a, b);
+  });
+}
+}  // namespace
+#endif
+
 void LiftoffAssembler::MergeFullStackWith(CacheState& target,
                                           const CacheState& source) {
   DCHECK_EQ(source.stack_height(), target.stack_height());
@@ -716,6 +740,9 @@ void LiftoffAssembler::MergeFullStackWith(CacheState& target,
   StackTransferRecipe transfers(this);
   for (uint32_t i = 0, e = source.stack_height(); i < e; ++i) {
     transfers.TransferStackSlot(target.stack_state[i], source.stack_state[i]);
+    DCHECK(!SlotInterference(target.stack_state[i],
+                             base::VectorOf(source.stack_state.data() + i + 1,
+                                            source.stack_height() - i - 1)));
   }
 
   // Full stack merging is only done for forward jumps, so we can just clear the
@@ -745,10 +772,21 @@ void LiftoffAssembler::MergeStackWith(CacheState& target, uint32_t arity,
   for (uint32_t i = 0; i < target_stack_base; ++i) {
     transfers.TransferStackSlot(target.stack_state[i],
                                 cache_state_.stack_state[i]);
+    DCHECK(!SlotInterference(
+        target.stack_state[i],
+        base::VectorOf(cache_state_.stack_state.data() + i + 1,
+                       target_stack_base - i - 1)));
+    DCHECK(!SlotInterference(
+        target.stack_state[i],
+        base::VectorOf(cache_state_.stack_state.data() + stack_base, arity)));
   }
   for (uint32_t i = 0; i < arity; ++i) {
     transfers.TransferStackSlot(target.stack_state[target_stack_base + i],
                                 cache_state_.stack_state[stack_base + i]);
+    DCHECK(!SlotInterference(
+        target.stack_state[i],
+        base::VectorOf(cache_state_.stack_state.data() + stack_base + i + 1,
+                       arity - i - 1)));
   }
 
   // Check whether the cached instance and/or memory start need to be moved to
@@ -1301,7 +1339,7 @@ std::ostream& operator<<(std::ostream& os, VarState slot) {
   os << name(slot.kind()) << ":";
   switch (slot.loc()) {
     case VarState::kStack:
-      return os << "s";
+      return os << "s0x" << std::hex << slot.offset() << std::dec;
     case VarState::kRegister:
       return os << slot.reg();
     case VarState::kIntConst:

src/wasm/baseline/liftoff-assembler.h

@@ -148,6 +148,7 @@ class LiftoffAssembler : public TurboAssembler {
     }
 
     int offset() const { return spill_offset_; }
+    void set_offset(int offset) { spill_offset_ = offset; }
 
     Register gp_reg() const { return reg().gp(); }
     DoubleRegister fp_reg() const { return reg().fp(); }
@@ -491,14 +492,18 @@ class LiftoffAssembler : public TurboAssembler {
   // stack, so that we can merge different values on the back-edge.
   void PrepareLoopArgs(int num);
 
-  int NextSpillOffset(ValueKind kind) {
-    int offset = TopSpillOffset() + SlotSizeForType(kind);
+  V8_INLINE static int NextSpillOffset(ValueKind kind, int top_spill_offset) {
+    int offset = top_spill_offset + SlotSizeForType(kind);
     if (NeedsAlignment(kind)) {
       offset = RoundUp(offset, SlotSizeForType(kind));
     }
     return offset;
   }
 
+  int NextSpillOffset(ValueKind kind) {
+    return NextSpillOffset(kind, TopSpillOffset());
+  }
+
   int TopSpillOffset() const {
     return cache_state_.stack_state.empty()
                ? StaticStackFrameSize()

test/mjsunit/regress/wasm/regress-12270.js

+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --liftoff-only
+
+d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
+
+(function Regress12270() {
+  print(arguments.callee.name);
+  let builder = new WasmModuleBuilder();
+  let sig = makeSig([], [kWasmF32, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64, kWasmF64]);
+  let sig_index = builder.addType(sig);
+  builder.addFunction("main", sig).addBody([
+      kExprI32Const, 0,
+      kExprIf, sig_index,
+        kExprF32Const, 0x0, 0x0, 0x0, 0x0,
+        kExprBlock, kWasmVoid,
+          kExprCallFunction, 0,
+          kExprBr, 1,
+        kExprEnd,
+        kExprUnreachable,
+      kExprElse,
+        kExprF32Const, 0x00, 0x00, 0x80, 0x3f, // 1.0
+        kExprF64Const, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xf0, 0x3f, // 1.0
+        kExprF64Const, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x40, // 2.0
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+        kExprF64Const, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+      kExprEnd
+  ]).exportFunc();
+  let instance = builder.instantiate();
+  assertEquals(1.0, instance.exports.main()[1]);
+})();