From efc26f9b2b934abbad92514d0d66e24d6b7c3aef Mon Sep 17 00:00:00 2001 From: "ulan@chromium.org" Date: Wed, 22 Aug 2012 14:27:11 +0000 Subject: [PATCH] Fix rounding in Uint8ClampedArray setter. According to Web IDL spec, we should round to the nearest integer, choosing the even integer if it lies halfway between two. R=yangguo@chromium.org,kbr@chromium.org BUG=v8:2294 Review URL: https://chromiumcodereview.appspot.com/10831409 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@12364 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/macro-assembler-arm.cc | 13 ++++--- src/ia32/assembler-ia32.cc | 10 ++++++ src/ia32/assembler-ia32.h | 1 + src/ia32/disasm-ia32.cc | 3 +- src/ia32/macro-assembler-ia32.cc | 5 +-- src/objects.cc | 2 +- src/platform.h | 18 ++++++++++ src/x64/macro-assembler-x64.cc | 6 +--- test/mjsunit/regress/regress-2294.js | 70 ++++++++++++++++++++++++++++++++++++ 9 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 test/mjsunit/regress/regress-2294.js diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc index 0be9722..4d7198b 100644 --- a/src/arm/macro-assembler-arm.cc +++ b/src/arm/macro-assembler-arm.cc @@ -3684,10 +3684,15 @@ void MacroAssembler::ClampDoubleToUint8(Register result_reg, // In 0-255 range, round and truncate. bind(&in_bounds); - Vmov(temp_double_reg, 0.5); - vadd(temp_double_reg, input_reg, temp_double_reg); - vcvt_u32_f64(temp_double_reg.low(), temp_double_reg); - vmov(result_reg, temp_double_reg.low()); + // Save FPSCR. + vmrs(ip); + // Set rounding mode to round to the nearest integer by clearing bits[23:22]. + bic(result_reg, ip, Operand(kVFPRoundingModeMask)); + vmsr(result_reg); + vcvt_s32_f64(input_reg.low(), input_reg, kFPSCRRounding); + vmov(result_reg, input_reg.low()); + // Restore FPSCR. + vmsr(ip); bind(&done); } diff --git a/src/ia32/assembler-ia32.cc b/src/ia32/assembler-ia32.cc index 4005123..ea68c50 100644 --- a/src/ia32/assembler-ia32.cc +++ b/src/ia32/assembler-ia32.cc @@ -1938,6 +1938,16 @@ void Assembler::cvttsd2si(Register dst, const Operand& src) { } +void Assembler::cvtsd2si(Register dst, XMMRegister src) { + ASSERT(CpuFeatures::IsEnabled(SSE2)); + EnsureSpace ensure_space(this); + EMIT(0xF2); + EMIT(0x0F); + EMIT(0x2D); + emit_sse_operand(dst, src); +} + + void Assembler::cvtsi2sd(XMMRegister dst, const Operand& src) { ASSERT(CpuFeatures::IsEnabled(SSE2)); EnsureSpace ensure_space(this); diff --git a/src/ia32/assembler-ia32.h b/src/ia32/assembler-ia32.h index 60b680f..f95e7b7 100644 --- a/src/ia32/assembler-ia32.h +++ b/src/ia32/assembler-ia32.h @@ -983,6 +983,7 @@ class Assembler : public AssemblerBase { // SSE2 instructions void cvttss2si(Register dst, const Operand& src); void cvttsd2si(Register dst, const Operand& src); + void cvtsd2si(Register dst, XMMRegister src); void cvtsi2sd(XMMRegister dst, Register src) { cvtsi2sd(dst, Operand(src)); } void cvtsi2sd(XMMRegister dst, const Operand& src); diff --git a/src/ia32/disasm-ia32.cc b/src/ia32/disasm-ia32.cc index 10dcb23..008fdde 100644 --- a/src/ia32/disasm-ia32.cc +++ b/src/ia32/disasm-ia32.cc @@ -1472,6 +1472,7 @@ int DisassemblerIA32::InstructionDecode(v8::internal::Vector out_buffer, switch (b2) { case 0x2A: mnem = "cvtsi2sd"; break; case 0x2C: mnem = "cvttsd2si"; break; + case 0x2D: mnem = "cvtsd2si"; break; case 0x51: mnem = "sqrtsd"; break; case 0x58: mnem = "addsd"; break; case 0x59: mnem = "mulsd"; break; @@ -1484,7 +1485,7 @@ int DisassemblerIA32::InstructionDecode(v8::internal::Vector out_buffer, if (b2 == 0x2A) { AppendToBuffer("%s %s,", mnem, NameOfXMMRegister(regop)); data += PrintRightOperand(data); - } else if (b2 == 0x2C) { + } else if (b2 == 0x2C || b2 == 0x2D) { AppendToBuffer("%s %s,", mnem, NameOfCPURegister(regop)); data += PrintRightXMMOperand(data); } else if (b2 == 0xC2) { diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index 83aef78..e81144a 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -134,10 +134,7 @@ void MacroAssembler::ClampDoubleToUint8(XMMRegister input_reg, Set(result_reg, Immediate(0)); ucomisd(input_reg, scratch_reg); j(below, &done, Label::kNear); - ExternalReference half_ref = ExternalReference::address_of_one_half(); - movdbl(scratch_reg, Operand::StaticVariable(half_ref)); - addsd(scratch_reg, input_reg); - cvttsd2si(result_reg, Operand(scratch_reg)); + cvtsd2si(result_reg, input_reg); test(result_reg, Immediate(0xFFFFFF00)); j(zero, &done, Label::kNear); Set(result_reg, Immediate(255)); diff --git a/src/objects.cc b/src/objects.cc index 64f5a45..9306eb9 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -11628,7 +11628,7 @@ Object* ExternalPixelArray::SetValue(uint32_t index, Object* value) { clamped_value = 255; } else { // Other doubles are rounded to the nearest integer. - clamped_value = static_cast(double_value + 0.5); + clamped_value = static_cast(lrint(double_value)); } } else { // Clamp undefined to zero (default). All other types have been diff --git a/src/platform.h b/src/platform.h index 14f4551..7a32229 100644 --- a/src/platform.h +++ b/src/platform.h @@ -71,6 +71,24 @@ int signbit(double x); int strncasecmp(const char* s1, const char* s2, int n); +inline int lrint(double flt) { + int intgr; +#if defined(V8_TARGET_ARCH_IA32) + __asm { + fld flt + fistp intgr + }; +#else + intgr = static_cast(flt + 0.5); + if ((intgr & 1) != 0 && intgr - flt == 0.5) { + // If the number is halfway between two integers, round to the even one. + intgr--; + } + return intgr; +#endif +} + + #endif // _MSC_VER // Random is missing on both Visual Studio and MinGW. diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc index 6bd3a1c..87ac078 100644 --- a/src/x64/macro-assembler-x64.cc +++ b/src/x64/macro-assembler-x64.cc @@ -2846,11 +2846,7 @@ void MacroAssembler::ClampDoubleToUint8(XMMRegister input_reg, xorps(temp_xmm_reg, temp_xmm_reg); ucomisd(input_reg, temp_xmm_reg); j(below, &done, Label::kNear); - uint64_t one_half = BitCast(0.5); - Set(temp_reg, one_half); - movq(temp_xmm_reg, temp_reg); - addsd(temp_xmm_reg, input_reg); - cvttsd2si(result_reg, temp_xmm_reg); + cvtsd2si(result_reg, input_reg); testl(result_reg, Immediate(0xFFFFFF00)); j(zero, &done, Label::kNear); Set(result_reg, 255); diff --git a/test/mjsunit/regress/regress-2294.js b/test/mjsunit/regress/regress-2294.js new file mode 100644 index 0000000..43ba10d --- /dev/null +++ b/test/mjsunit/regress/regress-2294.js @@ -0,0 +1,70 @@ +// Copyright 2012 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Flags: --allow-natives-syntax + +var clampedArray = new Uint8ClampedArray(10); + +function test() { + clampedArray[0] = 0.499; + assertEquals(0, clampedArray[0]); + clampedArray[0] = 0.5; + assertEquals(0, clampedArray[0]); + clampedArray[0] = 0.501; + assertEquals(1, clampedArray[0]); + clampedArray[0] = 1.499; + assertEquals(1, clampedArray[0]); + clampedArray[0] = 1.5; + assertEquals(2, clampedArray[0]); + clampedArray[0] = 1.501; + assertEquals(2, clampedArray[0]); + clampedArray[0] = 2.5; + assertEquals(2, clampedArray[0]); + clampedArray[0] = 3.5; + assertEquals(4, clampedArray[0]); + clampedArray[0] = 252.5; + assertEquals(252, clampedArray[0]); + clampedArray[0] = 253.5; + assertEquals(254, clampedArray[0]); + clampedArray[0] = 254.5; + assertEquals(254, clampedArray[0]); + clampedArray[0] = 256.5; + assertEquals(255, clampedArray[0]); + clampedArray[0] = -0.5; + assertEquals(0, clampedArray[0]); + clampedArray[0] = -1.5; + assertEquals(0, clampedArray[0]); + clampedArray[0] = 1000000000000; + assertEquals(255, clampedArray[0]); + clampedArray[0] = -1000000000000; + assertEquals(0, clampedArray[0]); +} + +test(); +test(); +%OptimizeFunctionOnNextCall(test); +test(); -- 2.7.4