Changeset 244238 in webkit

Timestamp:

Apr 12, 2019 8:04:18 PM (5 years ago)

Author:

sbarati@apple.com

Message:

r244079 logically broke shouldSpeculateInt52
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/int52-rand-function.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecode/SpeculatedType.h (modified) (2 diffs)
• Source/JavaScriptCore/dfg/DFGFixupPhase.cpp (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGNode.h (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp (modified) (2 diffs)
• Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

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

trunk/Source/JavaScriptCore/ChangeLog

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

trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h

@@ -343 +343 @@
 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));
 }
 
@@ -359 +359 @@
 {
     return !!value && (value & SpecInt52Any) == value;
+}
+
+inline bool isInt32OrInt52Speculation(SpeculatedType value)
+{
+    return !!value && (value & (SpecInt32Only | SpecInt52Any)) == value;
 }
 

trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

@@ -3225 +3225 @@
             break;
         case Int52RepUse:
-            if (isAnyInt52Speculation(variable->prediction()))
+            if (!isInt32Speculation(variable->prediction()) && isInt32OrInt52Speculation(variable->prediction()))
                 m_profitabilityChanged |= variable->mergeIsProfitableToUnbox(true);
             break;

trunk/Source/JavaScriptCore/dfg/DFGNode.h

@@ -2349 +2349 @@
     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());
     }
     

trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp

@@ -643 +643 @@
             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;
@@ -735 +734 @@
         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;
         }

trunk/Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp

@@ -182 +182 @@
     // 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());
 }