- Revision
- 244238
- Author
- sbar...@apple.com
- Date
- 2019-04-12 20:04:18 -0700 (Fri, 12 Apr 2019)
Log 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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (244237 => 244238)
--- trunk/JSTests/ChangeLog 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/JSTests/ChangeLog 2019-04-13 03:04:18 UTC (rev 244238)
@@ -1,3 +1,13 @@
+2019-04-12 Saam barati <sbar...@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 <ysuz...@apple.com>
[JSC] op_has_indexed_property should not assume subscript part is Uint32
Added: trunk/JSTests/microbenchmarks/int52-rand-function.js (0 => 244238)
--- trunk/JSTests/microbenchmarks/int52-rand-function.js (rev 0)
+++ trunk/JSTests/microbenchmarks/int52-rand-function.js 2019-04-13 03:04:18 UTC (rev 244238)
@@ -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();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (244237 => 244238)
--- trunk/Source/_javascript_Core/ChangeLog 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-04-13 03:04:18 UTC (rev 244238)
@@ -1,5 +1,48 @@
2019-04-12 Saam barati <sbar...@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 <sbar...@apple.com>
+
Unreviewed. Build fix after r244233.
* assembler/CPU.cpp:
Modified: trunk/Source/_javascript_Core/bytecode/SpeculatedType.h (244237 => 244238)
--- trunk/Source/_javascript_Core/bytecode/SpeculatedType.h 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/bytecode/SpeculatedType.h 2019-04-13 03:04:18 UTC (rev 244238)
@@ -342,12 +342,12 @@
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 @@
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;
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (244237 => 244238)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2019-04-13 03:04:18 UTC (rev 244238)
@@ -3224,7 +3224,7 @@
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:
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (244237 => 244238)
--- trunk/Source/_javascript_Core/dfg/DFGNode.h 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h 2019-04-13 03:04:18 UTC (rev 244238)
@@ -2348,7 +2348,24 @@
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()
Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (244237 => 244238)
--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2019-04-13 03:04:18 UTC (rev 244238)
@@ -642,8 +642,7 @@
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 @@
{
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: {
Modified: trunk/Source/_javascript_Core/dfg/DFGVariableAccessData.cpp (244237 => 244238)
--- trunk/Source/_javascript_Core/dfg/DFGVariableAccessData.cpp 2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/_javascript_Core/dfg/DFGVariableAccessData.cpp 2019-04-13 03:04:18 UTC (rev 244238)
@@ -181,7 +181,7 @@
// 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()
