<!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>[192190] 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/192190">192190</a></dd>
<dt>Author</dt> <dd>sbarati@apple.com</dd>
<dt>Date</dt> <dd>2015-11-09 16:39:13 -0800 (Mon, 09 Nov 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
https://bugs.webkit.org/show_bug.cgi?id=145295
Reviewed by Filip Pizlo.
This patch fixes PutStackSinkingPhase to no longer escape the stack
locations that LoadVarargs and ForwardVarargs write to. We used
to consider sinking PutStacks right before a LoadVarargs/ForwardVarargs
because we considered them uses of such stack locations. They aren't
uses of those stack locations, they unconditionally write to those
stack locations. Sinking PutStacks to these nodes was not needed before,
but seemed mostly innocent. But I ran into a problem with this while implementing
FTL try/catch where we would end up having to generate a value for a sunken PutStack
right before a LoadVarargs. This would cause us to issue a GetStack that loaded garbage that
was then forwarded into a Phi that was used as the source as the PutStack. This caused the
abstract interpreter to confuse itself on type information for the garbage GetStack
that was fed into the Phi, which would cause the abstract interpreter to then claim
that the basic block with the PutStack in it would never be reached. This isn't true, the
block would indeed be reached. The solution here is to be more precise about the
liveness of locals w.r.t LoadVarargs and ForwardVarargs.
* dfg/DFGPreciseLocalClobberize.h:
(JSC::DFG::PreciseLocalClobberizeAdaptor::PreciseLocalClobberizeAdaptor):
(JSC::DFG::PreciseLocalClobberizeAdaptor::write):
* dfg/DFGPutStackSinkingPhase.cpp:
* dfg/DFGSSACalculator.h:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGPreciseLocalClobberizeh">trunk/Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGPutStackSinkingPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGSSACalculatorh">trunk/Source/JavaScriptCore/dfg/DFGSSACalculator.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (192189 => 192190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-11-10 00:39:13 UTC (rev 192190)
</span><span class="lines">@@ -1,3 +1,32 @@
</span><ins>+2015-11-09 Saam barati <sbarati@apple.com>
+
+ DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
+ https://bugs.webkit.org/show_bug.cgi?id=145295
+
+ Reviewed by Filip Pizlo.
+
+ This patch fixes PutStackSinkingPhase to no longer escape the stack
+ locations that LoadVarargs and ForwardVarargs write to. We used
+ to consider sinking PutStacks right before a LoadVarargs/ForwardVarargs
+ because we considered them uses of such stack locations. They aren't
+ uses of those stack locations, they unconditionally write to those
+ stack locations. Sinking PutStacks to these nodes was not needed before,
+ but seemed mostly innocent. But I ran into a problem with this while implementing
+ FTL try/catch where we would end up having to generate a value for a sunken PutStack
+ right before a LoadVarargs. This would cause us to issue a GetStack that loaded garbage that
+ was then forwarded into a Phi that was used as the source as the PutStack. This caused the
+ abstract interpreter to confuse itself on type information for the garbage GetStack
+ that was fed into the Phi, which would cause the abstract interpreter to then claim
+ that the basic block with the PutStack in it would never be reached. This isn't true, the
+ block would indeed be reached. The solution here is to be more precise about the
+ liveness of locals w.r.t LoadVarargs and ForwardVarargs.
+
+ * dfg/DFGPreciseLocalClobberize.h:
+ (JSC::DFG::PreciseLocalClobberizeAdaptor::PreciseLocalClobberizeAdaptor):
+ (JSC::DFG::PreciseLocalClobberizeAdaptor::write):
+ * dfg/DFGPutStackSinkingPhase.cpp:
+ * dfg/DFGSSACalculator.h:
+
</ins><span class="cx"> 2015-11-09 Filip Pizlo <fpizlo@apple.com>
</span><span class="cx">
</span><span class="cx"> B3->Air lowering should support CCall
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGPreciseLocalClobberizeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h (192189 => 192190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h 2015-11-10 00:39:13 UTC (rev 192190)
</span><span class="lines">@@ -42,7 +42,7 @@
</span><span class="cx"> : m_graph(graph)
</span><span class="cx"> , m_node(node)
</span><span class="cx"> , m_read(read)
</span><del>- , m_write(write)
</del><ins>+ , m_unconditionalWrite(write)
</ins><span class="cx"> , m_def(def)
</span><span class="cx"> {
</span><span class="cx"> }
</span><span class="lines">@@ -70,7 +70,7 @@
</span><span class="cx"> // We expect stack writes to already be precisely characterized by DFG::clobberize().
</span><span class="cx"> if (heap.kind() == Stack) {
</span><span class="cx"> RELEASE_ASSERT(!heap.payload().isTop());
</span><del>- callIfAppropriate(m_write, VirtualRegister(heap.payload().value32()));
</del><ins>+ callIfAppropriate(m_unconditionalWrite, VirtualRegister(heap.payload().value32()));
</ins><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -155,7 +155,7 @@
</span><span class="cx"> Graph& m_graph;
</span><span class="cx"> Node* m_node;
</span><span class="cx"> const ReadFunctor& m_read;
</span><del>- const WriteFunctor& m_write;
</del><ins>+ const WriteFunctor& m_unconditionalWrite;
</ins><span class="cx"> const DefFunctor& m_def;
</span><span class="cx"> };
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGPutStackSinkingPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp (192189 => 192190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp 2015-11-10 00:39:13 UTC (rev 192190)
</span><span class="lines">@@ -108,31 +108,29 @@
</span><span class="cx"> if (verbose)
</span><span class="cx"> dataLog("Live at ", node, ": ", live, "\n");
</span><span class="cx">
</span><ins>+ Vector<VirtualRegister, 4> reads;
+ Vector<VirtualRegister, 4> writes;
</ins><span class="cx"> auto escapeHandler = [&] (VirtualRegister operand) {
</span><span class="cx"> if (operand.isHeader())
</span><span class="cx"> return;
</span><span class="cx"> if (verbose)
</span><span class="cx"> dataLog(" ", operand, " is live at ", node, "\n");
</span><del>- live.operand(operand) = true;
</del><ins>+ reads.append(operand);
</ins><span class="cx"> };
</span><del>-
- // FIXME: This might mishandle LoadVarargs and ForwardVarargs. It might make us
- // think that the locals being written are stack-live here. They aren't. This
- // should be harmless since we overwrite them anyway, but still, it's sloppy.
- // https://bugs.webkit.org/show_bug.cgi?id=145295
</del><ins>+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ writes.append(operand);
+ };
+
</ins><span class="cx"> preciseLocalClobberize(
</span><del>- m_graph, node, escapeHandler, escapeHandler,
- [&] (VirtualRegister operand, LazyNode source) {
- RELEASE_ASSERT(source.isNode());
</del><ins>+ m_graph, node, escapeHandler, writeHandler,
+ [&] (VirtualRegister, LazyNode) { });
</ins><span class="cx">
</span><del>- if (source.asNode() == node) {
- // This is a load. Ignore it.
- return;
- }
-
- RELEASE_ASSERT(node->op() == PutStack);
- live.operand(operand) = false;
- });
</del><ins>+ for (VirtualRegister operand : writes)
+ live.operand(operand) = false;
+ for (VirtualRegister operand : reads)
+ live.operand(operand) = true;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (live == liveAtHead[block])
</span><span class="lines">@@ -205,10 +203,13 @@
</span><span class="cx"> // Represents the fact that the original code would have done a PutStack but we haven't
</span><span class="cx"> // identified an operation that would have observed that PutStack.
</span><span class="cx"> //
</span><del>- // This code has some interesting quirks because of the fact that neither liveness nor
- // deferrals are very precise. They are only precise enough to be able to correctly tell us
- // when we may [sic] need to execute PutStacks. This means that they may report the need to
- // execute a PutStack in cases where we actually don't really need it, and that's totally OK.
</del><ins>+ // We need to be precise about liveness in this phase because not doing so
+ // could cause us to insert a PutStack before a node we thought may escape a
+ // value that it doesn't really escape. Sinking this PutStack above such a node may
+ // cause us to insert a GetStack that we forward to the Phi we're feeding into the
+ // sunken PutStack. Inserting such a GetStack could cause us to load garbage and
+ // can confuse the AI to claim untrue things (like that the program will exit when
+ // it really won't).
</ins><span class="cx"> BlockMap<Operands<FlushFormat>> deferredAtHead(m_graph);
</span><span class="cx"> BlockMap<Operands<FlushFormat>> deferredAtTail(m_graph);
</span><span class="cx">
</span><span class="lines">@@ -269,6 +270,10 @@
</span><span class="cx"> // A GetStack doesn't affect anything, since we know which local we are reading
</span><span class="cx"> // from.
</span><span class="cx"> continue;
</span><ins>+ } else if (node->op() == PutStack) {
+ VirtualRegister operand = node->stackAccessData()->local;
+ deferred.operand(operand) = node->stackAccessData()->format;
+ continue;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> auto escapeHandler = [&] (VirtualRegister operand) {
</span><span class="lines">@@ -279,19 +284,15 @@
</span><span class="cx"> // We will materialize just before any reads.
</span><span class="cx"> deferred.operand(operand) = DeadFlush;
</span><span class="cx"> };
</span><ins>+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ deferred.operand(operand) = DeadFlush;
+ };
</ins><span class="cx">
</span><span class="cx"> preciseLocalClobberize(
</span><del>- m_graph, node, escapeHandler, escapeHandler,
- [&] (VirtualRegister operand, LazyNode source) {
- RELEASE_ASSERT(source.isNode());
-
- if (source.asNode() == node) {
- // This is a load. Ignore it.
- return;
- }
-
- deferred.operand(operand) = node->stackAccessData()->format;
- });
</del><ins>+ m_graph, node, escapeHandler, writeHandler,
+ [&] (VirtualRegister, LazyNode) { });
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (deferred == deferredAtTail[block])
</span><span class="lines">@@ -351,13 +352,13 @@
</span><span class="cx"> indexToOperand.append(operand);
</span><span class="cx"> }
</span><span class="cx">
</span><del>- HashSet<Node*> putLocalsToSink;
</del><ins>+ HashSet<Node*> putStacksToSink;
</ins><span class="cx">
</span><span class="cx"> for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
</span><span class="cx"> for (Node* node : *block) {
</span><span class="cx"> switch (node->op()) {
</span><span class="cx"> case PutStack:
</span><del>- putLocalsToSink.add(node);
</del><ins>+ putStacksToSink.add(node);
</ins><span class="cx"> ssaCalculator.newDef(
</span><span class="cx"> operandToVariable.operand(node->stackAccessData()->local),
</span><span class="cx"> block, node->child1().node());
</span><span class="lines">@@ -496,9 +497,19 @@
</span><span class="cx">
</span><span class="cx"> deferred.operand(operand) = DeadFlush;
</span><span class="cx"> };
</span><del>-
</del><ins>+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ // LoadVarargs and ForwardVarargs are unconditional writes to the stack
+ // locations they claim to write to. They do not read from the stack
+ // locations they write to. This makes those stack locations dead right
+ // before a LoadVarargs/ForwardVarargs. This means we should never sink
+ // PutStacks right to this point.
+ RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ deferred.operand(operand) = DeadFlush;
+ };
+
</ins><span class="cx"> preciseLocalClobberize(
</span><del>- m_graph, node, escapeHandler, escapeHandler,
</del><ins>+ m_graph, node, escapeHandler, writeHandler,
</ins><span class="cx"> [&] (VirtualRegister, LazyNode) { });
</span><span class="cx"> break;
</span><span class="cx"> } }
</span><span class="lines">@@ -552,13 +563,11 @@
</span><span class="cx"> for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
</span><span class="cx"> Node* node = block->at(nodeIndex);
</span><span class="cx">
</span><del>- if (!putLocalsToSink.contains(node))
</del><ins>+ if (!putStacksToSink.contains(node))
</ins><span class="cx"> continue;
</span><span class="cx">
</span><span class="cx"> node->remove();
</span><span class="cx"> }
</span><del>-
- insertionSet.execute(block);
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (verbose) {
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGSSACalculatorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGSSACalculator.h (192189 => 192190)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGSSACalculator.h 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/JavaScriptCore/dfg/DFGSSACalculator.h 2015-11-10 00:39:13 UTC (rev 192190)
</span><span class="lines">@@ -91,10 +91,10 @@
</span><span class="cx"> // FIXME: Make it easier to do this, that doesn't involve rerunning GCSE.
</span><span class="cx"> // https://bugs.webkit.org/show_bug.cgi?id=136639
</span><span class="cx"> //
</span><del>-// 4.3) Insert Upsilons for each Phi in each successor block. Use the available values table to
-// decide the source value for each Phi's variable. Note that you could also use
-// SSACalculator::reachingDefAtTail() instead of the available values table, though your
-// local available values table is likely to be more efficient.
</del><ins>+// 4.3) Insert Upsilons at the end of the current block for the corresponding Phis in each successor block.
+// Use the available values table to decide the source value for each Phi's variable. Note that
+// you could also use SSACalculator::reachingDefAtTail() instead of the available values table,
+// though your local available values table is likely to be more efficient.
</ins><span class="cx"> //
</span><span class="cx"> // The most obvious use of SSACalculator is for the CPS->SSA conversion itself, but it's meant to
</span><span class="cx"> // also be used for SSA update and for things like the promotion of heap fields to local SSA
</span></span></pre>
</div>
</div>
</body>
</html>