Bug 1073910: Fix ARM's roundf codegen for negative numbers; r=mjrosenb
authorBenjamin Bouvier <benj@benj.me>
Thu, 11 Dec 2014 12:10:27 +0100
changeset 219271 c05bc6213e419b020c1d8a47dfd62bcf1e3f2c93
parent 219270 fc6714e329f020606bdec21742a19328ba4d3030
child 219272 dd24124194d5dcdd9f1e40ad3aa57f4f86b42132
push id10368
push userkwierso@gmail.com
push dateFri, 12 Dec 2014 01:38:39 +0000
treeherderfx-team@5288b15d22de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmjrosenb
bugs1073910
milestone37.0a1
Bug 1073910: Fix ARM's roundf codegen for negative numbers; r=mjrosenb
js/src/jit-test/tests/ion/bug1073910.js
js/src/jit/arm/Assembler-arm.cpp
js/src/jit/arm/MacroAssembler-arm.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug1073910.js
@@ -0,0 +1,20 @@
+function roundf(y) {
+    return Math.round(Math.fround(y));
+}
+
+var x = -1;
+assertEq(roundf(x), x);
+assertEq(roundf(x), x);
+
+var x = -2;
+assertEq(roundf(x), x);
+assertEq(roundf(x), x);
+
+var x = -1024;
+assertEq(roundf(x), x);
+
+var x = -14680050;
+assertEq(roundf(x), Math.fround(x));
+
+var x = -8388610;
+assertEq(roundf(x), Math.fround(x));
--- a/js/src/jit/arm/Assembler-arm.cpp
+++ b/js/src/jit/arm/Assembler-arm.cpp
@@ -2209,18 +2209,17 @@ Assembler::as_vxfer(Register vt1, Regist
         // should be performed on the lower or upper half of the double. Moving
         // a single to/from 2N/2N+1 isn't equivalent, since there are 32 single
         // registers, and 32 double registers so there is no way to encode the
         // last 16 double registers.
         sz = IsDouble;
         MOZ_ASSERT(idx == 0 || idx == 1);
         // If we are transferring a single half of the double then it must be
         // moving a VFP reg to a core reg.
-        if (vt2 == InvalidReg)
-            MOZ_ASSERT(f2c == FloatToCore);
+        MOZ_ASSERT_IF(vt2 == InvalidReg, f2c == FloatToCore);
         idx = idx << 21;
     } else {
         MOZ_ASSERT(idx == 0);
     }
 
     if (vt2 == InvalidReg) {
         return writeVFPInst(sz, WordTransfer | f2c | c |
                             RT(vt1) | maybeRN(vt2) | VN(vm) | idx);
--- a/js/src/jit/arm/MacroAssembler-arm.cpp
+++ b/js/src/jit/arm/MacroAssembler-arm.cpp
@@ -4604,72 +4604,87 @@ MacroAssemblerARMCompat::round(FloatRegi
 }
 
 void
 MacroAssemblerARMCompat::roundf(FloatRegister input, Register output, Label *bail, FloatRegister tmp)
 {
     Label handleZero;
     Label handleNeg;
     Label fin;
+
     // Do a compare based on the original value, then do most other things based
     // on the shifted value.
-    ma_vcmpz_f32(input);
-    // Since we already know the sign bit, flip all numbers to be positive,
-    // stored in tmp.
-    ma_vabs_f32(input, tmp);
-    as_vmrs(pc);
+    compareFloat(input, NoVFPRegister);
     ma_b(&handleZero, Assembler::Equal);
     ma_b(&handleNeg, Assembler::Signed);
+
     // NaN is always a bail condition, just bail directly.
     ma_b(bail, Assembler::Overflow);
 
     // The argument is a positive number, truncation is the path to glory; Since
     // it is known to be > 0.0, explicitly convert to a larger range, then a
     // value that rounds to INT_MAX is explicitly different from an argument
     // that clamps to INT_MAX.
 
     // Add the biggest number less than 0.5f (not 0.5f, because adding that to
     // the biggest number less than 0.5f would undesirably round up to 1), and
     // store the result into tmp.
     ma_vimm_f32(GetBiggestNumberLessThan(0.5f), ScratchFloat32Reg);
-    ma_vadd_f32(ScratchFloat32Reg, tmp, tmp);
-
+    ma_vadd_f32(ScratchFloat32Reg, input, tmp);
+
+    // Note: it doesn't matter whether x + .5 === x or not here, as it doesn't
+    // affect the semantics of the float to unsigned conversion (in particular,
+    // we are not applying any fixup after the operation).
     ma_vcvt_F32_U32(tmp, ScratchFloat32Reg.uintOverlay());
     ma_vxfer(VFPRegister(ScratchFloat32Reg).uintOverlay(), output);
     ma_mov(output, output, SetCond);
     ma_b(bail, Signed);
     ma_b(&fin);
 
     bind(&handleZero);
+
     // Move the whole float32 into the output reg, if it is non-zero, then the
     // original value was -0.0.
     as_vxfer(output, InvalidReg, input, FloatToCore, Always, 0);
     ma_cmp(output, Imm32(0));
     ma_b(bail, NonZero);
     ma_b(&fin);
 
     bind(&handleNeg);
 
     // Add 0.5 to negative numbers, storing the result into tmp.
+    ma_vneg_f32(input, tmp);
     ma_vimm_f32(0.5f, ScratchFloat32Reg);
-    ma_vadd_f32(ScratchFloat32Reg, tmp, tmp);
+    ma_vadd_f32(tmp, ScratchFloat32Reg, ScratchFloat32Reg);
+
+    // Adding 0.5 to a float input has chances to yield the wrong result, if
+    // the input is too large. In this case, skip the -1 adjustment made below.
+    compareFloat(ScratchFloat32Reg, tmp);
 
     // Negative case, negate, then start dancing. This number may be positive,
     // since we added 0.5.
-    ma_vcvt_F32_U32(tmp, ScratchFloat32Reg.uintOverlay());
-    ma_vxfer(VFPRegister(ScratchFloat32Reg).uintOverlay(), output);
+    // /!\ The conditional jump afterwards depends on these two instructions
+    //     *not* setting the status flags. They need to not change after the
+    //     comparison above.
+    ma_vcvt_F32_U32(ScratchFloat32Reg, tmp.uintOverlay());
+    ma_vxfer(VFPRegister(tmp).uintOverlay(), output);
+
+    Label flipSign;
+    ma_b(&flipSign, Equal);
 
     // -output is now a correctly rounded value, unless the original value was
     // exactly halfway between two integers, at which point, it has been rounded
     // away from zero, when it should be rounded towards \infty.
-    ma_vcvt_U32_F32(ScratchFloat32Reg.uintOverlay(), ScratchFloat32Reg);
-    compareFloat(ScratchFloat32Reg, tmp);
+    ma_vcvt_U32_F32(tmp.uintOverlay(), tmp);
+    compareFloat(tmp, ScratchFloat32Reg);
     ma_sub(output, Imm32(1), output, NoSetCond, Equal);
+
     // Negate the output. Since INT_MIN < -INT_MAX, even after adding 1, the
     // result will still be a negative number.
+    bind(&flipSign);
     ma_rsb(output, Imm32(0), output, SetCond);
 
     // If the result looks non-negative, then this value didn't actually fit
     // into the int range, and special handling is required, or it was zero,
     // which means the result is actually -0.0 which also requires special
     // handling.
     ma_b(bail, NotSigned);