Changeset 215482 in webkit

Timestamp:

Apr 18, 2017 3:34:14 PM (7 years ago)

Author:

mark.lam@apple.com

Message:

r211670 broke double to int conversion.
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/double-to-int32.js (added)
• JSTests/stress/to-int32-sensible2.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/MathCommon.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/MathCommon.h (modified) (4 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -2223 +2223 @@
         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);

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -11810 +11810 @@
         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);
         

trunk/Source/JavaScriptCore/runtime/MathCommon.cpp

@@ -468 +468 @@
 }
 
-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)

trunk/Source/JavaScriptCore/runtime/MathCommon.h

@@ -26 +26 @@
 #pragma once
 
+#include "CPU.h"
 #include <cmath>
 #include <wtf/Optional.h>
@@ -34 +35 @@
 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()
@@ -84 +84 @@
     // 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;
 
@@ -101 +108 @@
     // 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;