[Liftoff] Don't force unrelated stack slots into registers

When initializing the stack state at a merge point, don't force all
stack slots into registers. Allow constants to stay constants as long
as they are not part of the merge. Otherwise we might break assumptions
of outer blocks which then try to merge a register into a constant and
fail.
Also, add some documentation to {InitMergeStackSlot} to document the
intent of the implementation.

R=titzer@chromium.org

Bug: v8:784050, v8:6600
Change-Id: I3a4c83b446909027be075d3207cb7c748a6b1aad
Reviewed-on: https://chromium-review.googlesource.com/766353Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49423}

src/wasm/baseline/liftoff-assembler.cc

@@ -94,8 +94,8 @@ class StackTransferRecipe {
           // There is a cycle. Spill one register, then continue.
           Register spill_reg = register_moves.back().src;
           asm_->Spill(next_spill_slot, spill_reg);
-          // Remember to reload that register later.
-          LoadStackSlot(spill_reg, next_spill_slot);
+          // Remember to reload into the destination register later.
+          LoadStackSlot(register_moves.back().dst, next_spill_slot);
           DCHECK_EQ(1, src_reg_use_count[spill_reg.code()]);
           src_reg_use_count[spill_reg.code()] = 0;
           ++next_spill_slot;
@@ -189,38 +189,58 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
   DCHECK(stack_state.empty());
   DCHECK_GE(source.stack_height(), stack_base);
   stack_state.resize(stack_base + arity);
-  auto slot = stack_state.begin();
-
-  // TODO(clemensh): Avoid using registers which are already in use in source.
-  PinnedRegisterScope used_regs;
-  auto InitStackSlot = [this, &used_regs, &slot](const VarState& src) {
-    Register reg = no_reg;
-    if (src.is_reg() && !used_regs.has(src.reg)) {
-      reg = src.reg;
-    } else if (has_unused_register(used_regs)) {
-      reg = unused_register(used_regs);
+
+  // |------locals------|--(in between)--|--(discarded)--|----merge----|
+  //  <-- num_locals -->                 ^stack_base      <-- arity -->
+
+  // First, initialize merge slots and locals. Keep them in the registers which
+  // are being used in {source}, but avoid using a register multiple times. Use
+  // unused registers where necessary and possible.
+  for (int range = 0; range < 2; ++range) {
+    auto src_idx = range ? 0 : source.stack_state.size() - arity;
+    auto src_end = range ? num_locals : source.stack_state.size();
+    auto dst_idx = range ? 0 : stack_state.size() - arity;
+    for (; src_idx < src_end; ++src_idx, ++dst_idx) {
+      auto& dst = stack_state[dst_idx];
+      auto& src = source.stack_state[src_idx];
+      Register reg = no_reg;
+      if (src.is_reg() && is_free(src.reg)) {
+        reg = src.reg;
+      } else if (has_unused_register()) {
+        reg = unused_register();
+      } else {
+        // Keep this a stack slot (which is the initial value).
+        DCHECK(src.is_stack());
+        DCHECK(dst.is_stack());
+        continue;
+      }
+      dst = VarState(reg);
+      inc_used(reg);
+    }
+  }
+  // Last, initialize the section in between. Here, constants are allowed, but
+  // registers which are already used for the merge region or locals must be
+  // spilled.
+  for (uint32_t i = num_locals; i < stack_base; ++i) {
+    auto& dst = stack_state[i];
+    auto& src = source.stack_state[i];
+    if (src.is_reg()) {
+      if (is_used(src.reg)) {
+        // Keep this a stack slot (which is the initial value).
+        DCHECK(dst.is_stack());
+        continue;
+      }
+      dst = VarState(src.reg);
+      inc_used(src.reg);
+    } else if (src.is_const()) {
+      dst = src;
     } else {
       // Keep this a stack slot (which is the initial value).
-      DCHECK(slot->is_stack());
-      ++slot;
-      return;
+      DCHECK(src.is_stack());
+      DCHECK(dst.is_stack());
+      continue;
     }
-    *slot = VarState(reg);
-    ++slot;
-    inc_used(reg);
-    used_regs.pin(reg);
-  };
-
-  auto source_slot = source.stack_state.begin();
-  for (uint32_t i = 0; i < stack_base; ++i, ++source_slot) {
-    InitStackSlot(*source_slot);
-  }
-  DCHECK_GE(source.stack_height(), stack_base + arity);
-  source_slot = source.stack_state.end() - arity;
-  for (uint32_t i = 0; i < arity; ++i, ++source_slot) {
-    InitStackSlot(*source_slot);
   }
-  DCHECK_EQ(slot, stack_state.end());
   last_spilled_reg = source.last_spilled_reg;
 }
 

src/wasm/baseline/liftoff-assembler.h

@@ -118,6 +118,9 @@ class LiftoffAssembler : public TurboAssembler {
     uint32_t stack_base = 0;
     Register last_spilled_reg = Register::from_code<0>();
 
+    // InitMerge: Initialize this CacheState from the {source} cache state, but
+    // make sure that other code paths can still jump here (i.e. avoid constants
+    // in the locals or the merge region as specified by {arity}).
     // TODO(clemensh): Don't copy the full parent state (this makes us N^2).
     void InitMerge(const CacheState& source, uint32_t num_locals,
                    uint32_t arity);

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

+// Copyright 2017 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.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+var builder = new WasmModuleBuilder();
+builder.addFunction('test', kSig_v_v)
+    .addBodyWithEnd([
+      kExprI32Const,    0x0,   // const 0
+      kExprI32Const,    0x0,   // const 0
+      kExprBrIf,        0x00,  // br depth=0
+      kExprLoop,        0x7f,  // loop i32
+      kExprBlock,       0x7f,  // block i32
+      kExprI32Const,    0x0,   // const 0
+      kExprBr,          0x00,  // br depth=0
+      kExprEnd,                // end
+      kExprBr,          0x00,  // br depth=0
+      kExprEnd,                // end
+      kExprUnreachable,        // unreachable
+      kExprEnd,                // end
+    ])
+    .exportFunc();
+builder.instantiate();