[Liftoff] Fewer pinned registers on store

On ia32, we were pinning too many registers, resulting in no unpinned
byte registers left (we only have three byte registers since {ebx}
is reserved for the root register).
It turns out that on most paths, we don't actually need to pin any
registers, since {Store} is often the last call for an operation (like
any store or set_global). If registers need to be pinned, only pass
those that must be kept alive across the {Store}. This allows to
compute a more narrow set of pinned registers on demand inside {Store}.

Plus minor drive-by changes.

R=titzer@chromium.org

Bug: chromium:894374, chromium:894307, v8:6600
Change-Id: Ic4d7131784c193dc7a2abf0e504d9973f6d5c5f1
Reviewed-on: https://chromium-review.googlesource.com/c/1275819
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56587}

src/wasm/baseline/ia32/liftoff-assembler-ia32.h

@@ -321,7 +321,13 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
       if (src.gp().is_byte_register()) {
         mov_b(dst_op, src.gp());
       } else {
-        Register byte_src = GetUnusedRegister(liftoff::kByteRegs, pinned).gp();
+        // We know that {src} is not a byte register, so the only pinned byte
+        // registers (beside the outer {pinned}) are {dst_addr} and potentially
+        // {offset_reg}.
+        LiftoffRegList pinned_byte = pinned | LiftoffRegList::ForRegs(dst_addr);
+        if (offset_reg != no_reg) pinned_byte.set(offset_reg);
+        Register byte_src =
+            GetUnusedRegister(liftoff::kByteRegs, pinned_byte).gp();
         mov(byte_src, src.gp());
         mov_b(dst_op, byte_src);
       }

src/wasm/baseline/liftoff-compiler.cc

@@ -1210,7 +1210,7 @@ class LiftoffCompiler {
     LiftoffRegister addr = GetGlobalBaseAndOffset(global, pinned, &offset);
     LiftoffRegister reg = pinned.set(__ PopToRegister(pinned));
     StoreType type = StoreType::ForValueType(global->type);
-    __ Store(addr.gp(), no_reg, offset, reg, type, pinned, nullptr, true);
+    __ Store(addr.gp(), no_reg, offset, reg, type, {}, nullptr, true);
   }
 
   void Unreachable(FullDecoder* decoder) {
@@ -1556,7 +1556,9 @@ class LiftoffCompiler {
     LiftoffRegister addr = pinned.set(__ GetUnusedRegister(kGpReg, pinned));
     LOAD_INSTANCE_FIELD(addr, MemoryStart, kPointerSize);
     uint32_t protected_store_pc = 0;
-    __ Store(addr.gp(), index.gp(), offset, value, type, pinned,
+    LiftoffRegList outer_pinned;
+    if (FLAG_trace_wasm_memory) outer_pinned.set(index);
+    __ Store(addr.gp(), index.gp(), offset, value, type, outer_pinned,
              &protected_store_pc, true);
     if (env_->use_trap_handler) {
       AddOutOfLineTrap(decoder->position(),

src/wasm/baseline/liftoff-register.h

@@ -253,6 +253,10 @@ class LiftoffRegList {
     return LiftoffRegList(regs_ & other.regs_);
   }
 
+  constexpr LiftoffRegList operator|(const LiftoffRegList other) const {
+    return LiftoffRegList(regs_ | other.regs_);
+  }
+
   constexpr bool operator==(const LiftoffRegList other) const {
     return regs_ == other.regs_;
   }

src/wasm/baseline/mips/liftoff-assembler-mips.h

@@ -401,16 +401,16 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
                              StoreType type, LiftoffRegList pinned,
                              uint32_t* protected_store_pc, bool is_store_mem) {
   Register dst = no_reg;
+  MemOperand dst_op = MemOperand(dst_addr, offset_imm);
   if (offset_reg != no_reg) {
     dst = GetUnusedRegister(kGpReg, pinned).gp();
     emit_ptrsize_add(dst, dst_addr, offset_reg);
+    dst_op = MemOperand(dst, offset_imm);
   }
-  MemOperand dst_op = (offset_reg != no_reg) ? MemOperand(dst, offset_imm)
-                                             : MemOperand(dst_addr, offset_imm);
 
 #if defined(V8_TARGET_BIG_ENDIAN)
   if (is_store_mem) {
-    pinned.set(dst_op.rm());
+    pinned |= LiftoffRegList::ForRegs(dst_op.rm(), src);
     LiftoffRegister tmp = GetUnusedRegister(src.reg_class(), pinned);
     // Save original value.
     Move(tmp, src, type.value_type());
@@ -442,15 +442,11 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
       TurboAssembler::Usw(src.gp(), dst_op);
       break;
     case StoreType::kI64Store: {
-      MemOperand dst_op =
-          (offset_reg != no_reg)
-              ? MemOperand(dst, offset_imm + liftoff::kLowWordOffset)
-              : MemOperand(dst_addr, offset_imm + liftoff::kLowWordOffset);
-      MemOperand dst_op_upper =
-          (offset_reg != no_reg)
-              ? MemOperand(dst, offset_imm + liftoff::kHighWordOffset)
-              : MemOperand(dst_addr, offset_imm + liftoff::kHighWordOffset);
-      TurboAssembler::Usw(src.low_gp(), dst_op);
+      MemOperand dst_op_lower(dst_op.rm(),
+                              offset_imm + liftoff::kLowWordOffset);
+      MemOperand dst_op_upper(dst_op.rm(),
+                              offset_imm + liftoff::kHighWordOffset);
+      TurboAssembler::Usw(src.low_gp(), dst_op_lower);
       TurboAssembler::Usw(src.high_gp(), dst_op_upper);
       break;
     }

src/wasm/baseline/mips64/liftoff-assembler-mips64.h

@@ -338,12 +338,12 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
                              StoreType type, LiftoffRegList pinned,
                              uint32_t* protected_store_pc, bool is_store_mem) {
   Register dst = no_reg;
+  MemOperand dst_op = MemOperand(dst_addr, offset_imm);
   if (offset_reg != no_reg) {
     dst = GetUnusedRegister(kGpReg, pinned).gp();
     emit_ptrsize_add(dst, dst_addr, offset_reg);
+    dst_op = MemOperand(dst, offset_imm);
   }
-  MemOperand dst_op = (offset_reg != no_reg) ? MemOperand(dst, offset_imm)
-                                             : MemOperand(dst_addr, offset_imm);
 
 #if defined(V8_TARGET_BIG_ENDIAN)
   if (is_store_mem) {

src/wasm/baseline/x64/liftoff-assembler-x64.h

@@ -256,7 +256,7 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr,
 
 void LiftoffAssembler::Store(Register dst_addr, Register offset_reg,
                              uint32_t offset_imm, LiftoffRegister src,
-                             StoreType type, LiftoffRegList pinned,
+                             StoreType type, LiftoffRegList /* pinned */,
                              uint32_t* protected_store_pc, bool is_store_mem) {
   if (emit_debug_code() && offset_reg != no_reg) {
     AssertZeroExtended(offset_reg);

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

+// Copyright 2018 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');
+
+const builder = new WasmModuleBuilder();
+builder.addMemory(16, 32, false);
+const sig = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
+builder.addFunction(undefined, sig)
+  .addBodyWithEnd([
+    kExprMemorySize, 0,
+    kExprI32Const, 0,
+    kExprI64Const, 0,
+    kExprI64StoreMem8, 0, 0,
+    kExprEnd,
+  ]);
+builder.addExport('main', 0);
+builder.instantiate();  // shouldn't crash