[Liftoff] Fix conditional spilling

On float comparisons, we need a scratch byte register for the setcc
instruction, and if none is available, we spill. But this spilling code
is skipped if one of the operands is NaN. The cache state is updated
however, so following code assumes that the spill happened.
This CL fixes this by spilling before checking for NaN, such that the
spilling code is always executed.

R=titzer@chromium.org

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

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

@@ -873,19 +873,31 @@ void LiftoffAssembler::emit_cond_jump(Condition cond, Label* label,
 }
 
 namespace liftoff {
-inline void setcc_32(LiftoffAssembler* assm, Condition cond, Register dst) {
-  Register tmp_byte_reg = dst;
-  // Only the lower 4 registers can be addressed as 8-bit registers.
-  if (!dst.is_byte_register()) {
-    LiftoffRegList pinned = LiftoffRegList::ForRegs(dst);
-    // {GetUnusedRegister()} may insert move instructions to spill registers to
-    // the stack. This is OK because {mov} does not change the status flags.
-    tmp_byte_reg = assm->GetUnusedRegister(liftoff::kByteRegs, pinned).gp();
-  }
 
+// Get a temporary byte register, using {candidate} if possible.
+// Might spill, but always keeps status flags intact.
+inline Register GetTmpByteRegister(LiftoffAssembler* assm, Register candidate) {
+  if (candidate.is_byte_register()) return candidate;
+  LiftoffRegList pinned = LiftoffRegList::ForRegs(candidate);
+  // {GetUnusedRegister()} may insert move instructions to spill registers to
+  // the stack. This is OK because {mov} does not change the status flags.
+  return assm->GetUnusedRegister(liftoff::kByteRegs, pinned).gp();
+}
+
+// Setcc into dst register, given a scratch byte register (might be the same as
+// dst). Never spills.
+inline void setcc_32_no_spill(LiftoffAssembler* assm, Condition cond,
+                              Register dst, Register tmp_byte_reg) {
   assm->setcc(cond, tmp_byte_reg);
   assm->movzx_b(dst, tmp_byte_reg);
 }
+
+// Setcc into dst register (no contraints). Might spill.
+inline void setcc_32(LiftoffAssembler* assm, Condition cond, Register dst) {
+  Register tmp_byte_reg = GetTmpByteRegister(assm, dst);
+  setcc_32_no_spill(assm, cond, dst, tmp_byte_reg);
+}
+
 }  // namespace liftoff
 
 void LiftoffAssembler::emit_i32_eqz(Register dst, Register src) {
@@ -931,6 +943,10 @@ inline Condition cond_make_unsigned(Condition cond) {
 void LiftoffAssembler::emit_i64_set_cond(Condition cond, Register dst,
                                          LiftoffRegister lhs,
                                          LiftoffRegister rhs) {
+  // Get the tmp byte register out here, such that we don't conditionally spill
+  // (this cannot be reflected in the cache state).
+  Register tmp_byte_reg = liftoff::GetTmpByteRegister(this, dst);
+
   // For signed i64 comparisons, we still need to use unsigned comparison for
   // the low word (the only bit carrying signedness information is the MSB in
   // the high word).
@@ -945,11 +961,11 @@ void LiftoffAssembler::emit_i64_set_cond(Condition cond, Register dst,
   if (unsigned_cond != cond) {
     // If the condition predicate for the low differs from that for the high
     // word, emit a separete setcc sequence for the low word.
-    liftoff::setcc_32(this, unsigned_cond, dst);
+    liftoff::setcc_32_no_spill(this, unsigned_cond, dst, tmp_byte_reg);
     jmp(&cont);
   }
   bind(&setcc);
-  liftoff::setcc_32(this, cond, dst);
+  liftoff::setcc_32_no_spill(this, cond, dst, tmp_byte_reg);
   bind(&cont);
 }
 
@@ -959,8 +975,12 @@ void LiftoffAssembler::emit_f32_set_cond(Condition cond, Register dst,
   Label cont;
   Label not_nan;
 
+  // Get the tmp byte register out here, such that we don't conditionally spill
+  // (this cannot be reflected in the cache state).
+  Register tmp_byte_reg = liftoff::GetTmpByteRegister(this, dst);
+
   ucomiss(lhs, rhs);
-  // IF PF is one, one of the operands was Nan. This needs special handling.
+  // If PF is one, one of the operands was Nan. This needs special handling.
   j(parity_odd, &not_nan, Label::kNear);
   // Return 1 for f32.ne, 0 for all other cases.
   if (cond == not_equal) {
@@ -971,7 +991,7 @@ void LiftoffAssembler::emit_f32_set_cond(Condition cond, Register dst,
   jmp(&cont, Label::kNear);
   bind(&not_nan);
 
-  liftoff::setcc_32(this, cond, dst);
+  liftoff::setcc_32_no_spill(this, cond, dst, tmp_byte_reg);
   bind(&cont);
 }
 

test/mjsunit/regress/wasm/regress-7582.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.addGlobal(kWasmI32, 1);
+sig0 = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
+builder.addFunction(undefined, sig0)
+  .addBody([
+kExprF32Const, 0x01, 0x00, 0x00, 0x00,
+kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+kExprF32Eq,  // --> i32:0
+kExprF32Const, 0xc9, 0xc9, 0x69, 0xc9,
+kExprF32Const, 0xc9, 0xc9, 0xc9, 0x00,
+kExprF32Eq,  // --> i32:0 i32:0
+kExprIf, kWasmF32,
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+kExprElse,   // @32
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+  kExprEnd,   // --> i32:0 f32:0
+kExprF32Const, 0xc9, 0x00, 0x00, 0x00,
+kExprF32Const, 0xc9, 0xc9, 0xc9, 0x00,
+kExprF32Const, 0xc9, 0xc9, 0xa0, 0x00, // --> i32:0 f32:0 f32 f32 f32
+kExprF32Eq,  // --> i32:0 f32:0 f32 i32:0
+kExprIf, kWasmF32,
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+kExprElse,
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+  kExprEnd,  // --> i32:0 f32:0 f32 f32:0
+kExprF32Eq,  // --> i32:0 f32:0 i32:0
+kExprIf, kWasmF32,
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+kExprElse,
+  kExprF32Const, 0x00, 0x00, 0x00, 0x00,
+  kExprEnd,   // --> i32:0 f32:0 f32:0
+kExprF32Const, 0xc9, 0xc9, 0xff, 0xff,  // --> i32:0 f32:0 f32:0 f32
+kExprF32Eq,  // --> i32:0 f32:0 i32:0
+kExprDrop,
+kExprDrop, // --> i32:0
+kExprI32Const, 1, // --> i32:0 i32:1
+kExprI32GeU,  // --> i32:0
+          ]);
+builder.addExport('main', 0);
+const instance = builder.instantiate();
+assertEquals(0, instance.exports.main(1, 2, 3));