<!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>[191716] trunk/Source/JavaScriptCore</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/191716">191716</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-10-28 18:28:06 -0700 (Wed, 28 Oct 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>B3::LowerToAir should not duplicate Loads
https://bugs.webkit.org/show_bug.cgi?id=150651
Reviewed by Benjamin Poulain.
The instruction selector may decide to fuse two Values into one. This ordinarily only happens
if we haven't already emitted code that uses the Value and the Value has only one direct
user. Once we have emitted such code, we ensure that everyone knows that we have "locked" the
Value: we won't emit any more code for it in the future.
The optimization to fuse Loads was forgetting to do all of these things, and so generated
code would have a lot of duplicated Loads. That's bad and this change fixes that.
Ordinarily, this is far less tricky because the pattern matcher does this for us via
acceptInternals() and acceptInternalsLate(). I added a comment to this effect. I hope that we
won't need to do this manually very often.
Also found an uninitialized value bug in UseCounts. That was making all of this super hard to
debug.
* b3/B3IndexMap.h:
(JSC::B3::IndexMap::IndexMap):
(JSC::B3::IndexMap::resize):
(JSC::B3::IndexMap::operator[]):
* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::tmp):
(JSC::B3::Air::LowerToAir::canBeInternal):
(JSC::B3::Air::LowerToAir::commitInternal):
(JSC::B3::Air::LowerToAir::effectiveAddr):
(JSC::B3::Air::LowerToAir::loadAddr):
(JSC::B3::Air::LowerToAir::appendBinOp):
(JSC::B3::Air::LowerToAir::tryAppendStoreBinOp):
(JSC::B3::Air::LowerToAir::acceptInternals):
* b3/B3UseCounts.cpp:
(JSC::B3::UseCounts::UseCounts):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3IndexMaph">trunk/Source/JavaScriptCore/b3/B3IndexMap.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3LowerToAircpp">trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3UseCountscpp">trunk/Source/JavaScriptCore/b3/B3UseCounts.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (191715 => 191716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-10-29 01:24:33 UTC (rev 191715)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-10-29 01:28:06 UTC (rev 191716)
</span><span class="lines">@@ -1,3 +1,41 @@
</span><ins>+2015-10-28 Filip Pizlo <fpizlo@apple.com>
+
+ B3::LowerToAir should not duplicate Loads
+ https://bugs.webkit.org/show_bug.cgi?id=150651
+
+ Reviewed by Benjamin Poulain.
+
+ The instruction selector may decide to fuse two Values into one. This ordinarily only happens
+ if we haven't already emitted code that uses the Value and the Value has only one direct
+ user. Once we have emitted such code, we ensure that everyone knows that we have "locked" the
+ Value: we won't emit any more code for it in the future.
+
+ The optimization to fuse Loads was forgetting to do all of these things, and so generated
+ code would have a lot of duplicated Loads. That's bad and this change fixes that.
+
+ Ordinarily, this is far less tricky because the pattern matcher does this for us via
+ acceptInternals() and acceptInternalsLate(). I added a comment to this effect. I hope that we
+ won't need to do this manually very often.
+
+ Also found an uninitialized value bug in UseCounts. That was making all of this super hard to
+ debug.
+
+ * b3/B3IndexMap.h:
+ (JSC::B3::IndexMap::IndexMap):
+ (JSC::B3::IndexMap::resize):
+ (JSC::B3::IndexMap::operator[]):
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::tmp):
+ (JSC::B3::Air::LowerToAir::canBeInternal):
+ (JSC::B3::Air::LowerToAir::commitInternal):
+ (JSC::B3::Air::LowerToAir::effectiveAddr):
+ (JSC::B3::Air::LowerToAir::loadAddr):
+ (JSC::B3::Air::LowerToAir::appendBinOp):
+ (JSC::B3::Air::LowerToAir::tryAppendStoreBinOp):
+ (JSC::B3::Air::LowerToAir::acceptInternals):
+ * b3/B3UseCounts.cpp:
+ (JSC::B3::UseCounts::UseCounts):
+
</ins><span class="cx"> 2015-10-28 Mark Lam <mark.lam@apple.com>
</span><span class="cx">
</span><span class="cx"> JITSubGenerator::generateFastPath() does not need to be inlined.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3IndexMaph"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3IndexMap.h (191715 => 191716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3IndexMap.h 2015-10-29 01:24:33 UTC (rev 191715)
+++ trunk/Source/JavaScriptCore/b3/B3IndexMap.h 2015-10-29 01:28:06 UTC (rev 191716)
</span><span class="lines">@@ -41,12 +41,12 @@
</span><span class="cx"> public:
</span><span class="cx"> explicit IndexMap(size_t size = 0)
</span><span class="cx"> {
</span><del>- m_vector.resize(size);
</del><ins>+ m_vector.fill(Value(), size);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void resize(size_t size)
</span><span class="cx"> {
</span><del>- m_vector.resize(size);
</del><ins>+ m_vector.fill(Value(), size);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> Value& operator[](Key* key)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3LowerToAircpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp (191715 => 191716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2015-10-29 01:24:33 UTC (rev 191715)
+++ trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2015-10-29 01:28:06 UTC (rev 191716)
</span><span class="lines">@@ -115,6 +115,29 @@
</span><span class="cx"> return tmp;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ bool canBeInternal(Value* value)
+ {
+ // If one of the internal things has already been computed, then we don't want to cause
+ // it to be recomputed again.
+ if (valueToTmp[value])
+ return false;
+
+ // We require internals to have only one use - us.
+ if (useCounts[value] != 1)
+ return false;
+
+ return true;
+ }
+
+ // If you ask canBeInternal() and then construct something from that, and you commit to emitting
+ // that code, then you must commitInternal() on that value. This is tricky, and you only need to
+ // do it if you're pattern matching by hand rather than using the patterns language. Long story
+ // short, you should avoid this by using the pattern matcher to match patterns.
+ void commitInternal(Value* value)
+ {
+ locked.add(value);
+ }
+
</ins><span class="cx"> // This turns the given operand into an address.
</span><span class="cx"> Arg effectiveAddr(Value* address)
</span><span class="cx"> {
</span><span class="lines">@@ -150,6 +173,8 @@
</span><span class="cx">
</span><span class="cx"> Arg loadAddr(Value* loadValue)
</span><span class="cx"> {
</span><ins>+ if (!canBeInternal(loadValue))
+ return Arg();
</ins><span class="cx"> if (loadValue->opcode() == Load)
</span><span class="cx"> return addr(loadValue);
</span><span class="cx"> return Arg();
</span><span class="lines">@@ -214,6 +239,7 @@
</span><span class="cx"> if (commutativity == Commutative) {
</span><span class="cx"> Arg leftAddr = loadAddr(left);
</span><span class="cx"> if (isValidForm(opcode, leftAddr.kind(), Arg::Tmp)) {
</span><ins>+ commitInternal(left);
</ins><span class="cx"> append(relaxedMoveForType(currentValue->type()), tmp(right), result);
</span><span class="cx"> append(opcode, leftAddr, result);
</span><span class="cx"> return;
</span><span class="lines">@@ -222,6 +248,7 @@
</span><span class="cx">
</span><span class="cx"> Arg rightAddr = loadAddr(right);
</span><span class="cx"> if (isValidForm(opcode, rightAddr.kind(), Arg::Tmp)) {
</span><ins>+ commitInternal(right);
</ins><span class="cx"> append(relaxedMoveForType(currentValue->type()), tmp(left), result);
</span><span class="cx"> append(opcode, rightAddr, result);
</span><span class="cx"> return;
</span><span class="lines">@@ -253,15 +280,19 @@
</span><span class="cx"> Arg storeAddr = addr(currentValue);
</span><span class="cx"> ASSERT(storeAddr);
</span><span class="cx">
</span><ins>+ Value* loadValue;
</ins><span class="cx"> Value* otherValue;
</span><del>- if (addr(left) == storeAddr)
</del><ins>+ if (loadAddr(left) == storeAddr) {
+ loadValue = left;
</ins><span class="cx"> otherValue = right;
</span><del>- else if (commutativity == Commutative && addr(right) == storeAddr)
</del><ins>+ } else if (commutativity == Commutative && loadAddr(right) == storeAddr) {
+ loadValue = right;
</ins><span class="cx"> otherValue = left;
</span><del>- else
</del><ins>+ } else
</ins><span class="cx"> return false;
</span><span class="cx">
</span><span class="cx"> if (isValidForm(opcode, Arg::Imm, storeAddr.kind()) && imm(otherValue)) {
</span><ins>+ commitInternal(loadValue);
</ins><span class="cx"> append(opcode, imm(otherValue), storeAddr);
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="lines">@@ -269,6 +300,7 @@
</span><span class="cx"> if (!isValidForm(opcode, Arg::Tmp, storeAddr.kind()))
</span><span class="cx"> return false;
</span><span class="cx">
</span><ins>+ commitInternal(loadValue);
</ins><span class="cx"> append(opcode, tmp(otherValue), storeAddr);
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><span class="lines">@@ -420,15 +452,9 @@
</span><span class="cx"> template<typename... Arguments>
</span><span class="cx"> bool acceptInternals(Value* value, Arguments... arguments)
</span><span class="cx"> {
</span><del>- // If one of the internal things has already been computed, then we don't want to cause
- // it to be recomputed again.
- if (valueToTmp[value])
</del><ins>+ if (!canBeInternal(value))
</ins><span class="cx"> return false;
</span><span class="cx">
</span><del>- // We require internals to have only one use - us.
- if (useCounts[value] != 1)
- return false;
-
</del><span class="cx"> return acceptInternals(arguments...);
</span><span class="cx"> }
</span><span class="cx"> bool acceptInternals() { return true; }
</span><span class="lines">@@ -436,7 +462,7 @@
</span><span class="cx"> template<typename... Arguments>
</span><span class="cx"> void acceptInternalsLate(Value* value, Arguments... arguments)
</span><span class="cx"> {
</span><del>- locked.add(value);
</del><ins>+ commitInternal(value);
</ins><span class="cx"> acceptInternalsLate(arguments...);
</span><span class="cx"> }
</span><span class="cx"> void acceptInternalsLate() { }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3UseCountscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3UseCounts.cpp (191715 => 191716)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3UseCounts.cpp 2015-10-29 01:24:33 UTC (rev 191715)
+++ trunk/Source/JavaScriptCore/b3/B3UseCounts.cpp 2015-10-29 01:28:06 UTC (rev 191716)
</span><span class="lines">@@ -35,6 +35,8 @@
</span><span class="cx"> UseCounts::UseCounts(Procedure& procedure)
</span><span class="cx"> : m_counts(procedure.values().size())
</span><span class="cx"> {
</span><ins>+ for (Value* value : procedure.values())
+ ASSERT_UNUSED(value, !m_counts[value]);
</ins><span class="cx"> for (Value* value : procedure.values()) {
</span><span class="cx"> for (Value* child : value->children())
</span><span class="cx"> m_counts[child]++;
</span></span></pre>
</div>
</div>
</body>
</html>