<!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>[187513] 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/187513">187513</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-07-28 14:23:06 -0700 (Tue, 28 Jul 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>DFG::ArgumentsEliminationPhase has a redundant check for inserting CheckInBounds when converting GetByVal to GetStack in the inline non-varargs case
https://bugs.webkit.org/show_bug.cgi?id=147373
Reviewed by Mark Lam.
The code was doing a check for "index >= inlineCallFrame->arguments.size() - 1" in code where
safeToGetStack is true and we aren't in varargs context, but in a non-varargs context,
safeToGetStack can only be true if "index < inlineCallFrame->arguments.size() - 1".
When converting a GetByVal to GetStack, there are three possibilities:
1) Impossible to convert. This can happen if the GetByVal is out-of-bounds of the things we
know to have stored to the stack. For example, if we inline a function that does
"arguments[42]" at a call that passes no arguments.
2) Possible to convert, but we cannot prove statically that the GetByVal was in bounds. This
can happen for "arguments[42]" with no inline call frame (since we don't know statically
how many arguments we will be passed) or in a varargs call frame.
3) Possible to convert, and we know statically that the GetByVal is in bounds. This can
happen for "arguments[42]" if we have an inline call frame, and it's not a varargs call
frame, and we know that the caller passed 42 or more arguments.
The way the phase handles this is it first determines that we're not in case (1). This is
called safeToGetStack. safeToGetStack is true if we have case (2) or (3). For inline call
frames that have no varargs, this means that safeToGetStack is true exactly when the GetByVal
is in-bounds (i.e. case (3)).
But the phase was again doing a check for whether the index is in-bounds for non-varargs
inline call frames even when safeToGetStack was true. That check is redundant and should be
eliminated, since it makes the code confusing.
* dfg/DFGArgumentsEliminationPhase.cpp:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (187512 => 187513)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-07-28 21:19:28 UTC (rev 187512)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-07-28 21:23:06 UTC (rev 187513)
</span><span class="lines">@@ -1,5 +1,41 @@
</span><span class="cx"> 2015-07-28 Filip Pizlo <fpizlo@apple.com>
</span><span class="cx">
</span><ins>+ DFG::ArgumentsEliminationPhase has a redundant check for inserting CheckInBounds when converting GetByVal to GetStack in the inline non-varargs case
+ https://bugs.webkit.org/show_bug.cgi?id=147373
+
+ Reviewed by Mark Lam.
+
+ The code was doing a check for "index >= inlineCallFrame->arguments.size() - 1" in code where
+ safeToGetStack is true and we aren't in varargs context, but in a non-varargs context,
+ safeToGetStack can only be true if "index < inlineCallFrame->arguments.size() - 1".
+
+ When converting a GetByVal to GetStack, there are three possibilities:
+
+ 1) Impossible to convert. This can happen if the GetByVal is out-of-bounds of the things we
+ know to have stored to the stack. For example, if we inline a function that does
+ "arguments[42]" at a call that passes no arguments.
+
+ 2) Possible to convert, but we cannot prove statically that the GetByVal was in bounds. This
+ can happen for "arguments[42]" with no inline call frame (since we don't know statically
+ how many arguments we will be passed) or in a varargs call frame.
+
+ 3) Possible to convert, and we know statically that the GetByVal is in bounds. This can
+ happen for "arguments[42]" if we have an inline call frame, and it's not a varargs call
+ frame, and we know that the caller passed 42 or more arguments.
+
+ The way the phase handles this is it first determines that we're not in case (1). This is
+ called safeToGetStack. safeToGetStack is true if we have case (2) or (3). For inline call
+ frames that have no varargs, this means that safeToGetStack is true exactly when the GetByVal
+ is in-bounds (i.e. case (3)).
+
+ But the phase was again doing a check for whether the index is in-bounds for non-varargs
+ inline call frames even when safeToGetStack was true. That check is redundant and should be
+ eliminated, since it makes the code confusing.
+
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+
+2015-07-28 Filip Pizlo <fpizlo@apple.com>
+
</ins><span class="cx"> DFG::PutStackSinkingPhase should be more aggressive about its "no GetStack until put" rule
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=147371
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp (187512 => 187513)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2015-07-28 21:19:28 UTC (rev 187512)
+++ trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2015-07-28 21:23:06 UTC (rev 187513)
</span><span class="lines">@@ -452,8 +452,7 @@
</span><span class="cx"> arg += inlineCallFrame->stackOffset;
</span><span class="cx"> data = m_graph.m_stackAccessData.add(arg, FlushedJSValue);
</span><span class="cx">
</span><del>- if (!inlineCallFrame || inlineCallFrame->isVarargs()
- || index >= inlineCallFrame->arguments.size() - 1) {
</del><ins>+ if (!inlineCallFrame || inlineCallFrame->isVarargs()) {
</ins><span class="cx"> insertionSet.insertNode(
</span><span class="cx"> nodeIndex, SpecNone, CheckInBounds, node->origin,
</span><span class="cx"> node->child2(), Edge(getArrayLength(candidate), Int32Use));
</span></span></pre>
</div>
</div>
</body>
</html>