Fix three bugs with handling negative zero in the optimizing compiler.

* Bug fix for range analysis (contributed by Andy Wingo). Ranges of
double values have to include negative zero. Original code review:
 http://codereview.chromium.org/7514040/

* Fix a bug in optimized Math.round on ARM. When emitting minus-zero checks
we previously return a wrong result because of incorrect register assignment.

* Fix performance problem in IA32 and x64. Refine the checks
for minus zero and avoid unnecessary deoptimizations on Math.floor.

* Improve mjsunit test for Math.round to make sure we also
 get the optimized version of the code for each test case.
Review URL: http://codereview.chromium.org/7604028

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8877 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index 1ec5f00..ae9046d 100644 (file)
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -3014,19 +3014,18 @@ void LCodeGen::DoMathFloor(LUnaryMathOperation* instr) {
 void LCodeGen::DoMathRound(LUnaryMathOperation* instr) {
   DoubleRegister input = ToDoubleRegister(instr->InputAt(0));
   Register result = ToRegister(instr->result());
-  Register scratch1 = result;
-  Register scratch2 = scratch0();
+  Register scratch = scratch0();
   Label done, check_sign_on_zero;
 
   // Extract exponent bits.
-  __ vmov(scratch1, input.high());
-  __ ubfx(scratch2,
-          scratch1,
+  __ vmov(result, input.high());
+  __ ubfx(scratch,
+          result,
           HeapNumber::kExponentShift,
           HeapNumber::kExponentBits);
 
   // If the number is in ]-0.5, +0.5[, the result is +/- 0.
-  __ cmp(scratch2, Operand(HeapNumber::kExponentBias - 2));
+  __ cmp(scratch, Operand(HeapNumber::kExponentBias - 2));
   __ mov(result, Operand(0), LeaveCC, le);
   if (instr->hydrogen()->CheckFlag(HValue::kBailoutOnMinusZero)) {
     __ b(le, &check_sign_on_zero);
@@ -3036,19 +3035,19 @@ void LCodeGen::DoMathRound(LUnaryMathOperation* instr) {
 
   // The following conversion will not work with numbers
   // outside of ]-2^32, 2^32[.
-  __ cmp(scratch2, Operand(HeapNumber::kExponentBias + 32));
+  __ cmp(scratch, Operand(HeapNumber::kExponentBias + 32));
   DeoptimizeIf(ge, instr->environment());
 
   // Save the original sign for later comparison.
-  __ and_(scratch2, scratch1, Operand(HeapNumber::kSignMask));
+  __ and_(scratch, result, Operand(HeapNumber::kSignMask));
 
   __ Vmov(double_scratch0(), 0.5);
   __ vadd(input, input, double_scratch0());
 
   // Check sign of the result: if the sign changed, the input
   // value was in ]0.5, 0[ and the result should be -0.
-  __ vmov(scratch1, input.high());
-  __ eor(scratch1, scratch1, Operand(scratch2), SetCC);
+  __ vmov(result, input.high());
+  __ eor(result, result, Operand(scratch), SetCC);
   if (instr->hydrogen()->CheckFlag(HValue::kBailoutOnMinusZero)) {
     DeoptimizeIf(mi, instr->environment());
   } else {
@@ -3059,8 +3058,8 @@ void LCodeGen::DoMathRound(LUnaryMathOperation* instr) {
   __ EmitVFPTruncate(kRoundToMinusInf,
                      double_scratch0().low(),
                      input,
-                     scratch1,
-                     scratch2);
+                     result,
+                     scratch);
   DeoptimizeIf(ne, instr->environment());
   __ vmov(result, double_scratch0().low());
 
@@ -3069,8 +3068,8 @@ void LCodeGen::DoMathRound(LUnaryMathOperation* instr) {
     __ cmp(result, Operand(0));
     __ b(ne, &done);
     __ bind(&check_sign_on_zero);
-    __ vmov(scratch1, input.high());
-    __ tst(scratch1, Operand(HeapNumber::kSignMask));
+    __ vmov(scratch, input.high());
+    __ tst(scratch, Operand(HeapNumber::kSignMask));
     DeoptimizeIf(ne, instr->environment());
   }
   __ bind(&done);

diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index 2be2a03..5bea55a 100644 (file)
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -862,19 +862,10 @@ void HInstanceOf::PrintDataTo(StringStream* stream) {
 
 
 Range* HValue::InferRange() {
-  if (representation().IsTagged()) {
-    // Tagged values are always in int32 range when converted to integer,
-    // but they can contain -0.
-    Range* result = new Range();
-    result->set_can_be_minus_zero(true);
-    return result;
-  } else if (representation().IsNone()) {
-    return NULL;
-  } else {
-    // Untagged integer32 cannot be -0 and we don't compute ranges for
-    // untagged doubles.
-    return new Range();
-  }
+  // Untagged integer32 cannot be -0, all other representations can.
+  Range* result = new Range();
+  result->set_can_be_minus_zero(!representation().IsInteger32());
+  return result;
 }
 
 

diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc
index bbe8473..556b173 100644 (file)
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -2756,13 +2756,23 @@ void LCodeGen::DoMathFloor(LUnaryMathOperation* instr) {
   XMMRegister xmm_scratch = xmm0;
   Register output_reg = ToRegister(instr->result());
   XMMRegister input_reg = ToDoubleRegister(instr->value());
+  Label done;
+
+  // Deoptimize on negative numbers.
   __ xorps(xmm_scratch, xmm_scratch);  // Zero the register.
   __ ucomisd(input_reg, xmm_scratch);
+  DeoptimizeIf(below, instr->environment());
 
   if (instr->hydrogen()->CheckFlag(HValue::kBailoutOnMinusZero)) {
-    DeoptimizeIf(below_equal, instr->environment());
-  } else {
-    DeoptimizeIf(below, instr->environment());
+    // Check for negative zero.
+    Label positive_sign;
+    __ j(above, &positive_sign, Label::kNear);
+    __ movmskpd(output_reg, input_reg);
+    __ test(output_reg, Immediate(1));
+    DeoptimizeIf(not_zero, instr->environment());
+    __ Set(output_reg, Immediate(0));
+    __ jmp(&done, Label::kNear);
+    __ bind(&positive_sign);
   }
 
   // Use truncating instruction (OK because input is positive).
@@ -2771,6 +2781,7 @@ void LCodeGen::DoMathFloor(LUnaryMathOperation* instr) {
   // Overflow is signalled with minint.
   __ cmp(output_reg, 0x80000000u);
   DeoptimizeIf(equal, instr->environment());
+  __ bind(&done);
 }
 
 

diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
index 99f84b5..3cff9f1 100644 (file)
--- a/src/x64/lithium-codegen-x64.cc
+++ b/src/x64/lithium-codegen-x64.cc
@@ -2773,13 +2773,19 @@ void LCodeGen::DoMathFloor(LUnaryMathOperation* instr) {
     __ cmpl(output_reg, Immediate(0x80000000));
     DeoptimizeIf(equal, instr->environment());
   } else {
+    // Deoptimize on negative inputs.
     __ xorps(xmm_scratch, xmm_scratch);  // Zero the register.
     __ ucomisd(input_reg, xmm_scratch);
-
+    DeoptimizeIf(below, instr->environment());
     if (instr->hydrogen()->CheckFlag(HValue::kBailoutOnMinusZero)) {
-      DeoptimizeIf(below_equal, instr->environment());
-    } else {
-      DeoptimizeIf(below, instr->environment());
+      // Check for negative zero.
+      __ j(above, &positive_sign, Label::kNear);
+      __ movmskpd(output_reg, input_reg);
+      __ testq(output_reg, Immediate(1));
+      DeoptimizeIf(not_zero, instr->environment());
+      __ Set(output_reg, Immediate(0));
+      __ jmp(&done);
+      __ bind(&positive_sign);
     }
 
     // Use truncating instruction (OK because input is positive).
@@ -2789,6 +2795,7 @@ void LCodeGen::DoMathFloor(LUnaryMathOperation* instr) {
     __ cmpl(output_reg, Immediate(0x80000000));
     DeoptimizeIf(equal, instr->environment());
   }
+  __ bind(&done);
 }
 
 

diff --git a/test/mjsunit/math-floor.js b/test/mjsunit/math-floor.js
index 11f4cd7..c299203 100644 (file)
--- a/test/mjsunit/math-floor.js
+++ b/test/mjsunit/math-floor.js
@@ -51,6 +51,17 @@ function test() {
   testFloor(-Infinity, -Infinity);
   testFloor(NaN, NaN);
 
+  // Ensure that a negative zero coming from Math.floor is properly handled
+  // by other operations.
+  function ifloor(x) {
+    return 1 / Math.floor(x);
+  }
+  assertEquals(-Infinity, ifloor(-0));
+  assertEquals(-Infinity, ifloor(-0));
+  assertEquals(-Infinity, ifloor(-0));
+  %OptimizeFunctionOnNextCall(ifloor);
+  assertEquals(-Infinity, ifloor(-0));
+
   testFloor(0, 0.1);
   testFloor(0, 0.49999999999999994);
   testFloor(0, 0.5);
@@ -129,3 +140,19 @@ function test() {
 for (var i = 0; i < 500; i++) {
   test();
 }
+
+
+// Regression test for a bug where a negative zero coming from Math.floor
+// was not properly handled by other operations.
+function floorsum(i, n) {
+  var ret = Math.floor(n);
+  while (--i > 0) {
+    ret += Math.floor(n);
+  }
+  return ret;
+}
+assertEquals(-0, floorsum(1, -0));
+%OptimizeFunctionOnNextCall(floorsum);
+// The optimized function will deopt.  Run it with enough iterations to try
+// to optimize via OSR (triggering the bug).
+assertEquals(-0, floorsum(100000, -0));

diff --git a/test/mjsunit/math-round.js b/test/mjsunit/math-round.js
index 1366557..102c970 100644 (file)
--- a/test/mjsunit/math-round.js
+++ b/test/mjsunit/math-round.js
@@ -1,4 +1,4 @@
-// Copyright 2010 the V8 project authors. All rights reserved.
+// Copyright 2011 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:
@@ -27,10 +27,12 @@
 
 // Flags: --allow-natives-syntax
 
+var test_id = 0;
 function testRound(expect, input) {
-  function doRound(input) {
-    return Math.round(input);
-  }
+  // Make source code different on each invocation to make
+  // sure it gets optimized each time.
+  var doRound = new Function('input',
+                             '"' + (test_id++) + '";return Math.round(input)');
   assertEquals(expect, doRound(input));
   assertEquals(expect, doRound(input));
   assertEquals(expect, doRound(input));
@@ -44,6 +46,21 @@ testRound(Infinity, Infinity);
 testRound(-Infinity, -Infinity);
 testRound(NaN, NaN);
 
+// Regression test for a bug where a negative zero coming from Math.round
+// was not properly handled by other operations.
+function roundsum(i, n) {
+  var ret = Math.round(n);
+  while (--i > 0) {
+    ret += Math.round(n);
+  }
+  return ret;
+}
+assertEquals(-0, roundsum(1, -0));
+%OptimizeFunctionOnNextCall(roundsum);
+// The optimized function will deopt.  Run it with enough iterations to try
+// to optimize via OSR (triggering the bug).
+assertEquals(-0, roundsum(100000, -0));
+
 testRound(1, 0.5);
 testRound(1, 0.7);
 testRound(1, 1);