r244079 logically broke shouldSpeculateInt52
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Apr 2019 03:04:18 +0000 (03:04 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Apr 2019 03:04:18 +0000 (03:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196884

Reviewed by Yusuke Suzuki.

JSTests:

* microbenchmarks/int52-rand-function.js: Added.
(Math.random):

Source/JavaScriptCore:

In r244079, I changed shouldSpeculateInt52 to only return true
when the prediction is isAnyInt52Speculation(). However, it was
wrong to not to include SpecInt32 in this for two reasons:

1. We diligently write code that first checks if we should speculate Int32.
For example:
if (shouldSpeculateInt32()) ...
else if (shouldSpeculateInt52()) ...

It would be wrong not to fall back to Int52 if we're dealing with the union of
Int32 and Int52.

It would be a performance mistake to not include Int32 here because
data flow can easily tell us that we have variables that are the union
of Int32 and Int52 values. It's better to speculate Int52 than Double
in that situation.

2. We also write code where we ask if the inputs can be Int52, e.g, if
we know via profiling that an Add overflows, we may not emit an Int32 add.
However, we only emit such an add if both inputs can be Int52, and Int32
can trivially become Int52.

       This patch recovers the 0.5-1% regression r244079 caused on JetStream 2.

* bytecode/SpeculatedType.h:
(JSC::isInt32SpeculationForArithmetic):
(JSC::isInt32OrBooleanSpeculationForArithmetic):
(JSC::isInt32OrInt52Speculation):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::observeUseKindOnNode):
* dfg/DFGNode.h:
(JSC::DFG::Node::shouldSpeculateInt52):
* dfg/DFGPredictionPropagationPhase.cpp:
* dfg/DFGVariableAccessData.cpp:
(JSC::DFG::VariableAccessData::couldRepresentInt52Impl):

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

JSTests/ChangeLog
JSTests/microbenchmarks/int52-rand-function.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/SpeculatedType.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp

index bef9be9..ec1aa6f 100644 (file)
@@ -1,3 +1,13 @@
+2019-04-12  Saam barati  <sbarati@apple.com>
+
+        r244079 logically broke shouldSpeculateInt52
+        https://bugs.webkit.org/show_bug.cgi?id=196884
+
+        Reviewed by Yusuke Suzuki.
+
+        * microbenchmarks/int52-rand-function.js: Added.
+        (Math.random):
+
 2019-04-11  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] op_has_indexed_property should not assume subscript part is Uint32
diff --git a/JSTests/microbenchmarks/int52-rand-function.js b/JSTests/microbenchmarks/int52-rand-function.js
new file mode 100644 (file)
index 0000000..8ac8bbd
--- /dev/null
@@ -0,0 +1,20 @@
+let seed = 49734321;
+
+Math.random = (function() {
+    return function() {
+        // Robert Jenkins' 32 bit integer hash function.
+        seed = ((seed + 0x7ed55d16) + (seed << 12))  & 0xffffffff;
+        seed = ((seed ^ 0xc761c23c) ^ (seed >>> 19)) & 0xffffffff;
+        seed = ((seed + 0x165667b1) + (seed << 5))   & 0xffffffff;
+        seed = ((seed + 0xd3a2646c) ^ (seed << 9))   & 0xffffffff;
+        seed = ((seed + 0xfd7046c5) + (seed << 3))   & 0xffffffff;
+        seed = ((seed ^ 0xb55a4f09) ^ (seed >>> 16)) & 0xffffffff;
+        return (seed & 0xfffffff) / 0x10000000;
+    };
+})();
+
+noInline(Math.random);
+
+for (let i = 0; i < 10000000; ++i) {
+    Math.random();
+}
index 3f1a97e..1242135 100644 (file)
@@ -1,5 +1,48 @@
 2019-04-12  Saam barati  <sbarati@apple.com>
 
+        r244079 logically broke shouldSpeculateInt52
+        https://bugs.webkit.org/show_bug.cgi?id=196884
+
+        Reviewed by Yusuke Suzuki.
+
+        In r244079, I changed shouldSpeculateInt52 to only return true
+        when the prediction is isAnyInt52Speculation(). However, it was
+        wrong to not to include SpecInt32 in this for two reasons:
+
+        1. We diligently write code that first checks if we should speculate Int32.
+        For example:
+        if (shouldSpeculateInt32()) ... 
+        else if (shouldSpeculateInt52()) ...
+
+        It would be wrong not to fall back to Int52 if we're dealing with the union of
+        Int32 and Int52.
+
+        It would be a performance mistake to not include Int32 here because
+        data flow can easily tell us that we have variables that are the union
+        of Int32 and Int52 values. It's better to speculate Int52 than Double
+        in that situation.
+
+        2. We also write code where we ask if the inputs can be Int52, e.g, if
+        we know via profiling that an Add overflows, we may not emit an Int32 add.
+        However, we only emit such an add if both inputs can be Int52, and Int32
+        can trivially become Int52.
+
+       This patch recovers the 0.5-1% regression r244079 caused on JetStream 2.
+
+        * bytecode/SpeculatedType.h:
+        (JSC::isInt32SpeculationForArithmetic):
+        (JSC::isInt32OrBooleanSpeculationForArithmetic):
+        (JSC::isInt32OrInt52Speculation):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::observeUseKindOnNode):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::shouldSpeculateInt52):
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        * dfg/DFGVariableAccessData.cpp:
+        (JSC::DFG::VariableAccessData::couldRepresentInt52Impl):
+
+2019-04-12  Saam barati  <sbarati@apple.com>
+
         Unreviewed. Build fix after r244233.
 
         * assembler/CPU.cpp:
index d59bd03..af71b25 100644 (file)
@@ -342,12 +342,12 @@ inline bool isInt32OrBooleanSpeculation(SpeculatedType value)
 
 inline bool isInt32SpeculationForArithmetic(SpeculatedType value)
 {
-    return !(value & (SpecFullDouble | SpecInt52Any));
+    return !(value & (SpecFullDouble | SpecNonInt32AsInt52));
 }
 
 inline bool isInt32OrBooleanSpeculationForArithmetic(SpeculatedType value)
 {
-    return !(value & (SpecFullDouble | SpecInt52Any));
+    return !(value & (SpecFullDouble | SpecNonInt32AsInt52));
 }
 
 inline bool isInt32OrBooleanSpeculationExpectingDefined(SpeculatedType value)
@@ -360,6 +360,11 @@ inline bool isAnyInt52Speculation(SpeculatedType value)
     return !!value && (value & SpecInt52Any) == value;
 }
 
+inline bool isInt32OrInt52Speculation(SpeculatedType value)
+{
+    return !!value && (value & (SpecInt32Only | SpecInt52Any)) == value;
+}
+
 inline bool isIntAnyFormat(SpeculatedType value)
 {
     return !!value && (value & SpecIntAnyFormat) == value;
index e8aaf28..25d445d 100644 (file)
@@ -3224,7 +3224,7 @@ private:
                 m_profitabilityChanged |= variable->mergeIsProfitableToUnbox(true);
             break;
         case Int52RepUse:
-            if (isAnyInt52Speculation(variable->prediction()))
+            if (!isInt32Speculation(variable->prediction()) && isInt32OrInt52Speculation(variable->prediction()))
                 m_profitabilityChanged |= variable->mergeIsProfitableToUnbox(true);
             break;
         case CellUse:
index bb761b8..28b1d49 100644 (file)
@@ -2348,7 +2348,24 @@ public:
     
     bool shouldSpeculateInt52()
     {
-        return enableInt52() && isAnyInt52Speculation(prediction());
+        // We have to include SpecInt32Only here for two reasons:
+        // 1. We diligently write code that first checks if we should speculate Int32.
+        // For example:
+        // if (shouldSpeculateInt32()) ... 
+        // else if (shouldSpeculateInt52()) ...
+        // This means we it's totally valid to speculate Int52 when we're dealing
+        // with a type that's the union of Int32 and Int52.
+        //
+        // It would be a performance mistake to not include Int32 here because we obviously
+        // have variables that are the union of Int32 and Int52 values, and it's better
+        // to speculate Int52 than double in that situation.
+        //
+        // 2. We also write code where we ask if the inputs can be Int52, like if
+        // we know via profiling that an Add overflows, we may not emit an Int32 add.
+        // However, we only emit such an add if both inputs can be Int52, and Int32
+        // can trivially become Int52.
+        //
+        return enableInt52() && isInt32OrInt52Speculation(prediction());
     }
     
     bool shouldSpeculateDouble()
index 0fc70d4..e0f4e05 100644 (file)
@@ -642,8 +642,7 @@ private:
             SpeculatedType prediction = node->child1()->prediction();
             if (isDoubleSpeculation(prediction))
                 node->variableAccessData()->vote(VoteDouble, weight);
-            else if (!isFullNumberSpeculation(prediction)
-                || isInt32Speculation(prediction) || isAnyInt52Speculation(prediction))
+            else if (!isFullNumberSpeculation(prediction) || isInt32OrInt52Speculation(prediction))
                 node->variableAccessData()->vote(VoteValue, weight);
             break;
         }
@@ -734,7 +733,10 @@ private:
     {
         switch (m_currentNode->op()) {
         case JSConstant: {
-            setPrediction(speculationFromValue(m_currentNode->asJSValue()));
+            SpeculatedType type = speculationFromValue(m_currentNode->asJSValue());
+            if (type == SpecAnyIntAsDouble && enableInt52()) 
+                type = int52AwareSpeculationFromValue(m_currentNode->asJSValue());
+            setPrediction(type);
             break;
         }
         case DoubleConstant: {
index 7985e11..8ebffb4 100644 (file)
@@ -181,7 +181,7 @@ bool VariableAccessData::couldRepresentInt52Impl()
     
     // The argument-aware prediction -- which merges all of an (inlined or machine)
     // argument's variable access datas' predictions -- must possibly be Int52Any.
-    return isAnyInt52Speculation(argumentAwarePrediction());
+    return isInt32OrInt52Speculation(argumentAwarePrediction());
 }
 
 FlushFormat VariableAccessData::flushFormat()