From 2589ea08e33c653206a782ede7b08427d883d903 Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Wed, 21 Mar 2018 20:02:07 +0100 Subject: [PATCH] [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/973961 Reviewed-by: Ben Titzer Commit-Queue: Clemens Hammacher Cr-Commit-Position: refs/heads/master@{#52162} --- .../baseline/ia32/liftoff-assembler-ia32.h | 46 +++++++++++++----- test/mjsunit/regress/wasm/regress-7582.js | 48 +++++++++++++++++++ 2 files changed, 81 insertions(+), 13 deletions(-) create mode 100644 test/mjsunit/regress/wasm/regress-7582.js diff --git a/src/wasm/baseline/ia32/liftoff-assembler-ia32.h b/src/wasm/baseline/ia32/liftoff-assembler-ia32.h index 2f076c7cdf..94249eb377 100644 --- a/src/wasm/baseline/ia32/liftoff-assembler-ia32.h +++ b/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, ¬_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(¬_nan); - liftoff::setcc_32(this, cond, dst); + liftoff::setcc_32_no_spill(this, cond, dst, tmp_byte_reg); bind(&cont); } diff --git a/test/mjsunit/regress/wasm/regress-7582.js b/test/mjsunit/regress/wasm/regress-7582.js new file mode 100644 index 0000000000..476a0e18e8 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-7582.js @@ -0,0 +1,48 @@ +// 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));