r211670 broke double to int conversion.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Apr 2017 22:34:14 +0000 (22:34 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Apr 2017 22:34:14 +0000 (22:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170961
<rdar://problem/31687696>

Reviewed by Yusuke Suzuki.

JSTests:

* microbenchmarks/double-to-int32.js: Added.
* stress/to-int32-sensible2.js: Added.

Source/JavaScriptCore:

This is because operationToInt32SensibleSlow() assumes that left shifts of greater
than 31 bits on an 31-bit value will produce a 0.  However, the spec says that
"if the value of the right operand is negative or is greater or equal to the
number of bits in the promoted left operand, the behavior is undefined."
See http://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators.

This patch fixes this by restoring the check to prevent a shift of greater than
31 bits.  It also consolidates the optimization in operationToInt32SensibleSlow()
back into toInt32() so that we don't have 2 copies of the same code with only a
slight variation.

JSC benchmarks shows that performance is neutral with this patch.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileValueToInt32):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::sensibleDoubleToInt32):
* runtime/MathCommon.cpp:
(JSC::operationToInt32SensibleSlow): Deleted.
* runtime/MathCommon.h:
(JSC::toInt32):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@215482 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/microbenchmarks/double-to-int32.js [new file with mode: 0644]
JSTests/stress/to-int32-sensible2.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/MathCommon.cpp
Source/JavaScriptCore/runtime/MathCommon.h

index e444ee9..ecde294 100644 (file)
@@ -1,3 +1,14 @@
+2017-04-18  Mark Lam  <mark.lam@apple.com>
+
+        r211670 broke double to int conversion.
+        https://bugs.webkit.org/show_bug.cgi?id=170961
+        <rdar://problem/31687696>
+
+        Reviewed by Yusuke Suzuki.
+
+        * microbenchmarks/double-to-int32.js: Added.
+        * stress/to-int32-sensible2.js: Added.
+
 2017-04-18  Oleksandr Skachkov  <gskachkov@gmail.com>
 
         [ES6]. Implement Annex B.3.3 function hoisting rules for eval
diff --git a/JSTests/microbenchmarks/double-to-int32.js b/JSTests/microbenchmarks/double-to-int32.js
new file mode 100644 (file)
index 0000000..c85c2d3
--- /dev/null
@@ -0,0 +1,30 @@
+// This microbenchmarks validates that the fix in https://webkit.org/b/170961
+// does not regress the performance gains from r211670: <http://trac.webkit.org/changeset/211670>.
+// r211670 reduces the size of operationToInt32SensibleSlow() for handling double numbers with
+// binary exponent 31. Hence, this microbenchmark stresses doubleToIn32 conversion on
+// numbers with binary exponents in the vicinity of 31.
+
+let doubleValues = [
+    2.147483648e8, // exp = 27
+    2.147483648e9, // exp = 31
+    2.147483648e10, // exp = 34
+];
+
+function test(q, r, s, t, u, v, w, x, y, z) {
+    // Do a lot of double to int32 conversions to weigh more on the conversion.
+    return q|0 + r|0 + s|0 + t|0 + u|0 + v|0 + w|0 + x|0 + y|0 + z|0;
+}
+noInline(test);
+
+var result = 0;
+let length = doubleValues.length;
+for (var i = 0; i < 1000000; ++i) {
+    for (var j = 0; j < length; j++) {
+        var value = doubleValues[j];
+        result |= test(value, value, value, value, value, value, value, value, value, value);
+    }
+}
+
+if (result != -1932735284) {
+    throw "Bad result: " + result;
+}
diff --git a/JSTests/stress/to-int32-sensible2.js b/JSTests/stress/to-int32-sensible2.js
new file mode 100644 (file)
index 0000000..bbfe9be
--- /dev/null
@@ -0,0 +1,55 @@
+//@ runFTLNoCJIT
+
+var testCases = [
+    { value: -Number.MAX_VALUE, expected: 0 },
+    { value: Number.MAX_VALUE, expected: 0 },
+    { value: -Number.MIN_VALUE, expected: 0 },
+    { value: Number.MIN_VALUE, expected: 0 },
+    { value: -Infinity, expected: 0 },
+    { value: Infinity, expected: 0 },
+    { value: NaN, expected: 0 },
+    { value: -NaN, expected: 0 },
+    { value: 0, expected: 0 },
+    { value: -0, expected: 0 },
+    { value: 1, expected: 1 },
+    { value: -1, expected: -1 },
+    { value: 5, expected: 5 },
+    { value: -5, expected: -5 },
+
+    { value: 0x80000001, expected: -2147483647 },
+    { value: 0x80000000, expected: -2147483648 },
+    { value: 0x7fffffff, expected: 2147483647 },
+    { value: 0x7ffffffe, expected: 2147483646 },
+
+    { value: -2147483647, expected: -2147483647 },
+    { value: -2147483648, expected: -2147483648 },
+    { value: -2147483649, expected: 2147483647 },
+    { value: -2147483650, expected: 2147483646 },
+    
+    { value: 2147483646, expected: 2147483646 },
+    { value: 2147483647, expected: 2147483647 },
+    { value: 2147483648, expected: -2147483648 },
+    { value: 2147483649, expected: -2147483647 },
+];
+
+var numFailures = 0;
+for (var i = 0; i < testCases.length; i++) {
+    try {
+        var testCase = testCases[i];
+        var test = new Function("x", "y", "y = " + i + "; return x|0;");
+        noInline(test);
+
+        for (var k = 0; k < 10000; ++k) {
+            actual = test(testCase.value);
+            if (actual != testCase.expected)
+                throw ("FAILED: x|0 where x = " + testCase.value + ": expected " + testCase.expected + ", actual " + actual);
+        }
+    } catch (e) {
+        print(e);
+        numFailures++;
+    }
+}
+
+if (numFailures)
+    throw("Found " + numFailures + " failures");
+
index 813ef5f..f46acf3 100644 (file)
@@ -1,3 +1,33 @@
+2017-04-18  Mark Lam  <mark.lam@apple.com>
+
+        r211670 broke double to int conversion.
+        https://bugs.webkit.org/show_bug.cgi?id=170961
+        <rdar://problem/31687696>
+
+        Reviewed by Yusuke Suzuki.
+
+        This is because operationToInt32SensibleSlow() assumes that left shifts of greater
+        than 31 bits on an 31-bit value will produce a 0.  However, the spec says that
+        "if the value of the right operand is negative or is greater or equal to the
+        number of bits in the promoted left operand, the behavior is undefined."
+        See http://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators.
+
+        This patch fixes this by restoring the check to prevent a shift of greater than
+        31 bits.  It also consolidates the optimization in operationToInt32SensibleSlow()
+        back into toInt32() so that we don't have 2 copies of the same code with only a
+        slight variation.
+
+        JSC benchmarks shows that performance is neutral with this patch.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileValueToInt32):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::sensibleDoubleToInt32):
+        * runtime/MathCommon.cpp:
+        (JSC::operationToInt32SensibleSlow): Deleted.
+        * runtime/MathCommon.h:
+        (JSC::toInt32):
+
 2017-04-18  Oleksandr Skachkov  <gskachkov@gmail.com>
 
         [ES6]. Implement Annex B.3.3 function hoisting rules for eval
index bc33280..47e31e8 100644 (file)
@@ -2222,8 +2222,7 @@ void SpeculativeJIT::compileValueToInt32(Node* node)
         GPRReg gpr = result.gpr();
         JITCompiler::Jump notTruncatedToInteger = m_jit.branchTruncateDoubleToInt32(fpr, gpr, JITCompiler::BranchIfTruncateFailed);
         
-        addSlowPathGenerator(slowPathCall(notTruncatedToInteger, this,
-            hasSensibleDoubleToInt() ? operationToInt32SensibleSlow : operationToInt32, NeedToSpill, ExceptionCheckRequirement::CheckNotNeeded, gpr, fpr));
+        addSlowPathGenerator(slowPathCall(notTruncatedToInteger, this, operationToInt32, NeedToSpill, ExceptionCheckRequirement::CheckNotNeeded, gpr, fpr));
         
         int32Result(gpr, node);
         return;
index 1600b8f..fb0ae39 100644 (file)
@@ -11809,7 +11809,7 @@ private:
         
         LBasicBlock lastNext = m_out.appendTo(slowPath, continuation);
         ValueFromBlock slowResult = m_out.anchor(
-            m_out.call(Int32, m_out.operation(operationToInt32SensibleSlow), doubleValue));
+            m_out.call(Int32, m_out.operation(operationToInt32), doubleValue));
         m_out.jump(continuation);
         
         m_out.appendTo(continuation, lastNext);
index eb060a1..6f11975 100644 (file)
@@ -467,71 +467,6 @@ int32_t JIT_OPERATION operationToInt32(double value)
     return JSC::toInt32(value);
 }
 
-int32_t JIT_OPERATION operationToInt32SensibleSlow(double number)
-{
-    // This function is specialized `operationToInt32` for the slow case of
-    // the sensible double-to-int32 operation. It is available in x86.
-    //
-    // In the sensible double-to-int32, first we attempt to truncate the
-    // double value to int32 by using cvttsd2si_rr.
-    // According to the Intel's manual, cvttsd2si perform the following truncate
-    // operation.
-    //
-    // If src = NaN, +-Inf, or |(src)rz| > 0x7fffffff and (src)rz != 0x80000000,
-    // the result becomes 0x80000000. Otherwise, the operation succeeds.
-    // Note that ()rz is rouding towards zero.
-    //
-    // We call this slow case function when the above cvttsd2si fails. We check
-    // this condition by performing `result == 0x80000000`. So this function only
-    // accepts the following numbers.
-    //
-    //     NaN, +-Inf, |(src)rz| > 0x7fffffff.
-    //
-    // As a result, the exp of the double is always >= 31.
-    // This condition simplifies and speeds up the toInt32 implementation.
-    int64_t bits = WTF::bitwise_cast<int64_t>(number);
-    int32_t exp = (static_cast<int32_t>(bits >> 52) & 0x7ff) - 0x3ff;
-
-    // If exponent < 0 there will be no bits to the left of the decimal point
-    // after rounding; if the exponent is > 83 then no bits of precision can be
-    // left in the low 32-bit range of the result (IEEE-754 doubles have 52 bits
-    // of fractional precision).
-    // Note this case handles 0, -0, and all infinite, NaN, & denormal value.
-
-    // If exp < 0, truncate operation succeeds. So this function does not
-    // encounter that case. If exp > 83, it means exp >= 84. In that case,
-    // the following operation produces 0 for the result.
-    ASSERT(exp >= 0);
-
-    // Select the appropriate 32-bits from the floating point mantissa. If the
-    // exponent is 52 then the bits we need to select are already aligned to the
-    // lowest bits of the 64-bit integer representation of the number, no need
-    // to shift. If the exponent is greater than 52 we need to shift the value
-    // left by (exp - 52), if the value is less than 52 we need to shift right
-    // accordingly.
-    int32_t result = (exp > 52)
-        ? static_cast<int32_t>(bits << (exp - 52))
-        : static_cast<int32_t>(bits >> (52 - exp));
-
-    // IEEE-754 double precision values are stored omitting an implicit 1 before
-    // the decimal point; we need to reinsert this now. We may also the shifted
-    // invalid bits into the result that are not a part of the mantissa (the sign
-    // and exponent bits from the floatingpoint representation); mask these out.
-    //
-    // The important observation is that exp is always >= 31. So the above case
-    // is needed to be cared only when the exp == 31.
-    ASSERT(exp >= 31);
-    if (exp == 31) {
-        int32_t missingOne = 1 << exp;
-        result &= (missingOne - 1);
-        result += missingOne;
-    }
-
-    // If the input value was negative (we could test either 'number' or 'bits',
-    // but testing 'bits' is likely faster) invert the result appropriately.
-    return bits < 0 ? -result : result;
-}
-
 #if HAVE(ARM_IDIV_INSTRUCTIONS)
 static inline bool isStrictInt32(double value)
 {
index 4bc2f90..bae9ad2 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "CPU.h"
 #include <cmath>
 #include <wtf/Optional.h>
 
@@ -33,7 +34,6 @@ namespace JSC {
 const int32_t maxExponentForIntegerMathPow = 1000;
 double JIT_OPERATION operationMathPow(double x, double y) WTF_INTERNAL;
 int32_t JIT_OPERATION operationToInt32(double) WTF_INTERNAL;
-int32_t JIT_OPERATION operationToInt32SensibleSlow(double) WTF_INTERNAL;
 
 inline constexpr double maxSafeInteger()
 {
@@ -83,7 +83,14 @@ ALWAYS_INLINE int32_t toInt32(double number)
     // left in the low 32-bit range of the result (IEEE-754 doubles have 52 bits
     // of fractional precision).
     // Note this case handles 0, -0, and all infinite, NaN, & denormal value.
-    if (exp < 0 || exp > 83)
+
+    // We need to check exp > 83 because:
+    // 1. exp may be used as a left shift value below in (exp - 52), and
+    // 2. Left shift amounts that exceed 31 results in undefined behavior. See:
+    //    http://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators
+    //
+    // Using an unsigned comparison here also gives us a exp < 0 check for free.
+    if (static_cast<uint32_t>(exp) > 83u)
         return 0;
 
     // Select the appropriate 32-bits from the floating point mantissa. If the
@@ -100,7 +107,33 @@ ALWAYS_INLINE int32_t toInt32(double number)
     // the decimal point; we need to reinsert this now. We may also the shifted
     // invalid bits into the result that are not a part of the mantissa (the sign
     // and exponent bits from the floatingpoint representation); mask these out.
-    if (exp < 32) {
+    if (hasSensibleDoubleToInt() && (exp == 31)) {
+        // This is an optimization for when toInt32() is called in the slow path
+        // of a JIT operation. Currently, this optimization is only applicable for
+        // x86 ports.
+        //
+        // On x86, the fast path does a sensible double-to-int32 conversion, by
+        // first attempting to truncate the double value to int32 using the
+        // cvttsd2si_rr instruction. According to Intel's manual, cvttsd2si performs
+        // the following truncate operation:
+        //
+        //     If src = NaN, +-Inf, or |(src)rz| > 0x7fffffff and (src)rz != 0x80000000,
+        //     then the result becomes 0x80000000. Otherwise, the operation succeeds.
+        //
+        // Note that the ()rz notation means rounding towards zero.
+        // We'll call the slow case function only when the above cvttsd2si fails. The
+        // JIT code checks for fast path failure by checking if result == 0x80000000.
+        // Hence, the slow path will only see the following possible set of numbers:
+        //
+        //     NaN, +-Inf, or |(src)rz| > 0x7fffffff.
+        //
+        // As a result, the exp of the double is always >= 31. We can take advantage
+        // of this by specifically checking for (exp == 31) and give the compiler a
+        // chance to constant fold the operations below.
+        const int32_t missingOne = 1 << exp;
+        result &= missingOne - 1;
+        result += missingOne;
+    } else if (exp < 32) {
         int32_t missingOne = 1 << exp;
         result &= missingOne - 1;
         result += missingOne;