<!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>[160348] 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/160348">160348</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2013-12-09 21:52:24 -0800 (Mon, 09 Dec 2013)</dd>
</dl>
<h3>Log Message</h3>
<pre>Impose and enforce some basic rules of sanity for where Phi functions are allowed to occur and where their (optional) corresponding MovHints can be
https://bugs.webkit.org/show_bug.cgi?id=125480
Reviewed by Geoffrey Garen.
Previously, if you wanted to insert some speculation right after where a value was
produced, you'd get super confused if that value was produced by a Phi node. You can't
necessarily insert speculations after a Phi node because Phi nodes appear in this
special sequence of Phis and MovHints that establish the OSR exit state for a block.
So, you'd probably want to search for the next place where it's safe to insert things.
We already do this "search for beginning of next bytecode instruction" search by
looking at the next node that has a different CodeOrigin. But this would be hard for a
Phi because those Phis and MovHints have basically random CodeOrigins and they can all
have different CodeOrigins.
This change imposes some sanity for this situation:
- Phis must have unset CodeOrigins.
- In each basic block, all nodes that have unset CodeOrigins must come before all nodes
that have set CodeOrigins.
This all ends up working out just great because prior to this change we didn't have a
use for unset CodeOrigins. I think it's appropriate to make "unset CodeOrigin" mean
that we're in the prologue of a basic block.
It's interesting what this means for block merging, which we don't yet do in SSA.
Consider merging the edge A->B. One possibility is that the block merger is now
required to clean up Phi/Upsilons, and reascribe the MovHints to have the CodeOrigin of
the A's block terminal. But an answer that might be better is that the originless
nodes at the top of the B are just given the origin of the terminal and we keep the
Phis. That would require changing the above rules. We'll see how it goes, and what we
end up picking...
Overall, this special-things-at-the-top rule is analogous to what other SSA-based
compilers do. For example, LLVM has rules mandating that Phis appear at the top of a
block.
* bytecode/CodeOrigin.cpp:
(JSC::CodeOrigin::dump):
* dfg/DFGOSRExitBase.h:
(JSC::DFG::OSRExitBase::OSRExitBase):
* dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run):
* dfg/DFGValidate.cpp:
(JSC::DFG::Validate::validate):
(JSC::DFG::Validate::validateSSA):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodeCodeOrigincpp">trunk/Source/JavaScriptCore/bytecode/CodeOrigin.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGOSRExitBaseh">trunk/Source/JavaScriptCore/dfg/DFGOSRExitBase.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGSSAConversionPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGValidatecpp">trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (160347 => 160348)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2013-12-10 03:24:31 UTC (rev 160347)
+++ trunk/Source/JavaScriptCore/ChangeLog 2013-12-10 05:52:24 UTC (rev 160348)
</span><span class="lines">@@ -1,3 +1,53 @@
</span><ins>+2013-12-09 Filip Pizlo <fpizlo@apple.com>
+
+ Impose and enforce some basic rules of sanity for where Phi functions are allowed to occur and where their (optional) corresponding MovHints can be
+ https://bugs.webkit.org/show_bug.cgi?id=125480
+
+ Reviewed by Geoffrey Garen.
+
+ Previously, if you wanted to insert some speculation right after where a value was
+ produced, you'd get super confused if that value was produced by a Phi node. You can't
+ necessarily insert speculations after a Phi node because Phi nodes appear in this
+ special sequence of Phis and MovHints that establish the OSR exit state for a block.
+ So, you'd probably want to search for the next place where it's safe to insert things.
+ We already do this "search for beginning of next bytecode instruction" search by
+ looking at the next node that has a different CodeOrigin. But this would be hard for a
+ Phi because those Phis and MovHints have basically random CodeOrigins and they can all
+ have different CodeOrigins.
+
+ This change imposes some sanity for this situation:
+
+ - Phis must have unset CodeOrigins.
+
+ - In each basic block, all nodes that have unset CodeOrigins must come before all nodes
+ that have set CodeOrigins.
+
+ This all ends up working out just great because prior to this change we didn't have a
+ use for unset CodeOrigins. I think it's appropriate to make "unset CodeOrigin" mean
+ that we're in the prologue of a basic block.
+
+ It's interesting what this means for block merging, which we don't yet do in SSA.
+ Consider merging the edge A->B. One possibility is that the block merger is now
+ required to clean up Phi/Upsilons, and reascribe the MovHints to have the CodeOrigin of
+ the A's block terminal. But an answer that might be better is that the originless
+ nodes at the top of the B are just given the origin of the terminal and we keep the
+ Phis. That would require changing the above rules. We'll see how it goes, and what we
+ end up picking...
+
+ Overall, this special-things-at-the-top rule is analogous to what other SSA-based
+ compilers do. For example, LLVM has rules mandating that Phis appear at the top of a
+ block.
+
+ * bytecode/CodeOrigin.cpp:
+ (JSC::CodeOrigin::dump):
+ * dfg/DFGOSRExitBase.h:
+ (JSC::DFG::OSRExitBase::OSRExitBase):
+ * dfg/DFGSSAConversionPhase.cpp:
+ (JSC::DFG::SSAConversionPhase::run):
+ * dfg/DFGValidate.cpp:
+ (JSC::DFG::Validate::validate):
+ (JSC::DFG::Validate::validateSSA):
+
</ins><span class="cx"> 2013-12-08 Filip Pizlo <fpizlo@apple.com>
</span><span class="cx">
</span><span class="cx"> Reveal array bounds checks in DFG IR
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodeCodeOrigincpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/CodeOrigin.cpp (160347 => 160348)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/CodeOrigin.cpp 2013-12-10 03:24:31 UTC (rev 160347)
+++ trunk/Source/JavaScriptCore/bytecode/CodeOrigin.cpp 2013-12-10 05:52:24 UTC (rev 160348)
</span><span class="lines">@@ -59,6 +59,11 @@
</span><span class="cx">
</span><span class="cx"> void CodeOrigin::dump(PrintStream& out) const
</span><span class="cx"> {
</span><ins>+ if (!isSet()) {
+ out.print("<none>");
+ return;
+ }
+
</ins><span class="cx"> Vector<CodeOrigin> stack = inlineStack();
</span><span class="cx"> for (unsigned i = 0; i < stack.size(); ++i) {
</span><span class="cx"> if (i)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGOSRExitBaseh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGOSRExitBase.h (160347 => 160348)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGOSRExitBase.h 2013-12-10 03:24:31 UTC (rev 160347)
+++ trunk/Source/JavaScriptCore/dfg/DFGOSRExitBase.h 2013-12-10 05:52:24 UTC (rev 160348)
</span><span class="lines">@@ -48,6 +48,8 @@
</span><span class="cx"> , m_codeOrigin(origin)
</span><span class="cx"> , m_codeOriginForExitProfile(originForProfile)
</span><span class="cx"> {
</span><ins>+ ASSERT(m_codeOrigin.isSet());
+ ASSERT(m_codeOriginForExitProfile.isSet());
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> ExitKind m_kind;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGSSAConversionPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp (160347 => 160348)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp 2013-12-10 03:24:31 UTC (rev 160347)
+++ trunk/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp 2013-12-10 05:52:24 UTC (rev 160348)
</span><span class="lines">@@ -151,7 +151,7 @@
</span><span class="cx"> NodeFlags result = resultFor(format);
</span><span class="cx"> UseKind useKind = useKindFor(format);
</span><span class="cx">
</span><del>- node = m_insertionSet.insertNode(0, SpecNone, Phi, node->codeOrigin);
</del><ins>+ node = m_insertionSet.insertNode(0, SpecNone, Phi, CodeOrigin());
</ins><span class="cx"> node->mergeFlags(result);
</span><span class="cx"> RELEASE_ASSERT((node->flags() & NodeResultMask) == result);
</span><span class="cx">
</span><span class="lines">@@ -184,7 +184,7 @@
</span><span class="cx"> // the value was already on the stack.
</span><span class="cx"> } else {
</span><span class="cx"> m_insertionSet.insertNode(
</span><del>- 0, SpecNone, MovHint, node->codeOrigin, OpInfo(variable),
</del><ins>+ 0, SpecNone, MovHint, CodeOrigin(), OpInfo(variable),
</ins><span class="cx"> Edge(node));
</span><span class="cx"> }
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGValidatecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp (160347 => 160348)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp 2013-12-10 03:24:31 UTC (rev 160347)
+++ trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp 2013-12-10 05:52:24 UTC (rev 160348)
</span><span class="lines">@@ -191,7 +191,7 @@
</span><span class="cx"> break;
</span><span class="cx">
</span><span class="cx"> case SSA:
</span><del>- // FIXME: Implement SSA verification.
</del><ins>+ validateSSA();
</ins><span class="cx"> break;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -398,6 +398,40 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ void validateSSA()
+ {
+ // FIXME: Add more things here.
+ // https://bugs.webkit.org/show_bug.cgi?id=123471
+
+ for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex) {
+ BasicBlock* block = m_graph.block(blockIndex);
+ if (!block)
+ continue;
+
+ unsigned nodeIndex = 0;
+ for (; nodeIndex < block->size() && !block->at(nodeIndex)->codeOrigin.isSet(); nodeIndex++) { }
+
+ VALIDATE((block), nodeIndex < block->size());
+
+ for (; nodeIndex < block->size(); nodeIndex++)
+ VALIDATE((block->at(nodeIndex)), block->at(nodeIndex)->codeOrigin.isSet());
+
+ for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
+ Node* node = block->at(nodeIndex);
+ switch (node->op()) {
+ case Phi:
+ VALIDATE((node), !node->codeOrigin.isSet());
+ break;
+
+ default:
+ // FIXME: Add more things here.
+ // https://bugs.webkit.org/show_bug.cgi?id=123471
+ break;
+ }
+ }
+ }
+ }
+
</ins><span class="cx"> void checkOperand(
</span><span class="cx"> BasicBlock* block, Operands<size_t>& getLocalPositions,
</span><span class="cx"> Operands<size_t>& setLocalPositions, VirtualRegister operand)
</span></span></pre>
</div>
</div>
</body>
</html>