<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[244238] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/244238">244238</a></dd>
<dt>Author</dt> <dd>sbarati@apple.com</dd>
<dt>Date</dt> <dd>2019-04-12 20:04:18 -0700 (Fri, 12 Apr 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre><a href="http://trac.webkit.org/projects/webkit/changeset/244079">r244079</a> 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 <a href="http://trac.webkit.org/projects/webkit/changeset/244079">r244079</a>, 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 <a href="http://trac.webkit.org/projects/webkit/changeset/244079">r244079</a> 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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodeSpeculatedTypeh">trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGFixupPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGNodeh">trunk/Source/JavaScriptCore/dfg/DFGNode.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGPredictionPropagationPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGVariableAccessDatacpp">trunk/Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsmicrobenchmarksint52randfunctionjs">trunk/JSTests/microbenchmarks/int52-rand-function.js</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog  2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/JSTests/ChangeLog     2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -1,3 +1,13 @@
</span><ins>+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):
+
</ins><span class="cx"> 2019-04-11  Yusuke Suzuki  <ysuzuki@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [JSC] op_has_indexed_property should not assume subscript part is Uint32
</span></span></pre></div>
<a id="trunkJSTestsmicrobenchmarksint52randfunctionjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/microbenchmarks/int52-rand-function.js (0 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,20 @@
</span><ins>+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();
+}
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog    2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/ChangeLog       2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -1,5 +1,48 @@
</span><span class="cx"> 2019-04-12  Saam barati  <sbarati@apple.com>
</span><span class="cx"> 
</span><ins>+        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>
+
</ins><span class="cx">         Unreviewed. Build fix after r244233.
</span><span class="cx"> 
</span><span class="cx">         * assembler/CPU.cpp:
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodeSpeculatedTypeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h    2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/bytecode/SpeculatedType.h       2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -342,12 +342,12 @@
</span><span class="cx"> 
</span><span class="cx"> inline bool isInt32SpeculationForArithmetic(SpeculatedType value)
</span><span class="cx"> {
</span><del>-    return !(value & (SpecFullDouble | SpecInt52Any));
</del><ins>+    return !(value & (SpecFullDouble | SpecNonInt32AsInt52));
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> inline bool isInt32OrBooleanSpeculationForArithmetic(SpeculatedType value)
</span><span class="cx"> {
</span><del>-    return !(value & (SpecFullDouble | SpecInt52Any));
</del><ins>+    return !(value & (SpecFullDouble | SpecNonInt32AsInt52));
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> inline bool isInt32OrBooleanSpeculationExpectingDefined(SpeculatedType value)
</span><span class="lines">@@ -360,6 +360,11 @@
</span><span class="cx">     return !!value && (value & SpecInt52Any) == value;
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+inline bool isInt32OrInt52Speculation(SpeculatedType value)
+{
+    return !!value && (value & (SpecInt32Only | SpecInt52Any)) == value;
+}
+
</ins><span class="cx"> inline bool isIntAnyFormat(SpeculatedType value)
</span><span class="cx"> {
</span><span class="cx">     return !!value && (value & SpecIntAnyFormat) == value;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGFixupPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp        2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp   2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -3224,7 +3224,7 @@
</span><span class="cx">                 m_profitabilityChanged |= variable->mergeIsProfitableToUnbox(true);
</span><span class="cx">             break;
</span><span class="cx">         case Int52RepUse:
</span><del>-            if (isAnyInt52Speculation(variable->prediction()))
</del><ins>+            if (!isInt32Speculation(variable->prediction()) && isInt32OrInt52Speculation(variable->prediction()))
</ins><span class="cx">                 m_profitabilityChanged |= variable->mergeIsProfitableToUnbox(true);
</span><span class="cx">             break;
</span><span class="cx">         case CellUse:
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGNodeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGNode.h (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGNode.h        2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/dfg/DFGNode.h   2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -2348,7 +2348,24 @@
</span><span class="cx">     
</span><span class="cx">     bool shouldSpeculateInt52()
</span><span class="cx">     {
</span><del>-        return enableInt52() && isAnyInt52Speculation(prediction());
</del><ins>+        // 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());
</ins><span class="cx">     }
</span><span class="cx">     
</span><span class="cx">     bool shouldSpeculateDouble()
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGPredictionPropagationPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp        2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp   2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -642,8 +642,7 @@
</span><span class="cx">             SpeculatedType prediction = node->child1()->prediction();
</span><span class="cx">             if (isDoubleSpeculation(prediction))
</span><span class="cx">                 node->variableAccessData()->vote(VoteDouble, weight);
</span><del>-            else if (!isFullNumberSpeculation(prediction)
-                || isInt32Speculation(prediction) || isAnyInt52Speculation(prediction))
</del><ins>+            else if (!isFullNumberSpeculation(prediction) || isInt32OrInt52Speculation(prediction))
</ins><span class="cx">                 node->variableAccessData()->vote(VoteValue, weight);
</span><span class="cx">             break;
</span><span class="cx">         }
</span><span class="lines">@@ -734,7 +733,10 @@
</span><span class="cx">     {
</span><span class="cx">         switch (m_currentNode->op()) {
</span><span class="cx">         case JSConstant: {
</span><del>-            setPrediction(speculationFromValue(m_currentNode->asJSValue()));
</del><ins>+            SpeculatedType type = speculationFromValue(m_currentNode->asJSValue());
+            if (type == SpecAnyIntAsDouble && enableInt52()) 
+                type = int52AwareSpeculationFromValue(m_currentNode->asJSValue());
+            setPrediction(type);
</ins><span class="cx">             break;
</span><span class="cx">         }
</span><span class="cx">         case DoubleConstant: {
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGVariableAccessDatacpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp (244237 => 244238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp        2019-04-13 00:05:27 UTC (rev 244237)
+++ trunk/Source/JavaScriptCore/dfg/DFGVariableAccessData.cpp   2019-04-13 03:04:18 UTC (rev 244238)
</span><span class="lines">@@ -181,7 +181,7 @@
</span><span class="cx">     
</span><span class="cx">     // The argument-aware prediction -- which merges all of an (inlined or machine)
</span><span class="cx">     // argument's variable access datas' predictions -- must possibly be Int52Any.
</span><del>-    return isAnyInt52Speculation(argumentAwarePrediction());
</del><ins>+    return isInt32OrInt52Speculation(argumentAwarePrediction());
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> FlushFormat VariableAccessData::flushFormat()
</span></span></pre>
</div>
</div>

</body>
</html>