<!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>[207163] 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/207163">207163</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-10-11 13:37:51 -0700 (Tue, 11 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>B3->Air lowering needs the same defenses in effectiveAddr() that it has in tryAppendLea()
https://bugs.webkit.org/show_bug.cgi?id=163264
Reviewed by Mark Lam.
When writing the lea patch (<a href="http://trac.webkit.org/projects/webkit/changeset/207039">r207039</a>), I was very careful about how I convert a Shl into a
BaseIndex scale. But I forgot to check if the older code for creating BaseIndexes for
effectiveAddr() got this right. It turns out that the older code missed the <<32 corner
case.
It's sad that the two paths can't share all of their code, but it's somewhat inevitable due
to how matching an address and matching a lea have to do very different things. Matching a
lea means creating an instruction that is distinct from other instructions to do multiple
math operations at once. Matching an address means making some instruction do extra work
for free. Also, address matching can take advantage of the fact that the offset is already
associated with the memory operation by strength reduction - lea matching can't do this; it
has to figure out Add(@value, $const) on its own. This change makes the situation slightly
more sane by adding a scaleForShl() helper that handles this weird case. It's based on the
new Shl handling from <a href="http://trac.webkit.org/projects/webkit/changeset/207039">r207039</a>, and exposes it as an API for effectiveAddr() to use.
The testLoadBaseIndexShift32() used to crash. I don't think that this case affects JS
content, since <<32 is such a bizarre outlier. I don't think we even have a path along
which the FTL would emit a 64-bit <<32. It probably won't even affect WebAssembly since
that uses 32-bit pointers, so we won't see 64-bit <<32 in effectiveAddr() there.
* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::scaleForShl):
(JSC::B3::Air::LowerToAir::effectiveAddr):
(JSC::B3::Air::LowerToAir::tryAppendLea):
(JSC::B3::Air::LowerToAir::crossesInterference): Deleted.
* b3/testb3.cpp:
(JSC::B3::testLoadBaseIndexShift2):
(JSC::B3::testLoadBaseIndexShift32):
(JSC::B3::run):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3LowerToAircpp">trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3testb3cpp">trunk/Source/JavaScriptCore/b3/testb3.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (207162 => 207163)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-10-11 20:37:51 UTC (rev 207163)
</span><span class="lines">@@ -1,3 +1,40 @@
</span><ins>+2016-10-11 Filip Pizlo <fpizlo@apple.com>
+
+ B3->Air lowering needs the same defenses in effectiveAddr() that it has in tryAppendLea()
+ https://bugs.webkit.org/show_bug.cgi?id=163264
+
+ Reviewed by Mark Lam.
+
+ When writing the lea patch (r207039), I was very careful about how I convert a Shl into a
+ BaseIndex scale. But I forgot to check if the older code for creating BaseIndexes for
+ effectiveAddr() got this right. It turns out that the older code missed the <<32 corner
+ case.
+
+ It's sad that the two paths can't share all of their code, but it's somewhat inevitable due
+ to how matching an address and matching a lea have to do very different things. Matching a
+ lea means creating an instruction that is distinct from other instructions to do multiple
+ math operations at once. Matching an address means making some instruction do extra work
+ for free. Also, address matching can take advantage of the fact that the offset is already
+ associated with the memory operation by strength reduction - lea matching can't do this; it
+ has to figure out Add(@value, $const) on its own. This change makes the situation slightly
+ more sane by adding a scaleForShl() helper that handles this weird case. It's based on the
+ new Shl handling from r207039, and exposes it as an API for effectiveAddr() to use.
+
+ The testLoadBaseIndexShift32() used to crash. I don't think that this case affects JS
+ content, since <<32 is such a bizarre outlier. I don't think we even have a path along
+ which the FTL would emit a 64-bit <<32. It probably won't even affect WebAssembly since
+ that uses 32-bit pointers, so we won't see 64-bit <<32 in effectiveAddr() there.
+
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::scaleForShl):
+ (JSC::B3::Air::LowerToAir::effectiveAddr):
+ (JSC::B3::Air::LowerToAir::tryAppendLea):
+ (JSC::B3::Air::LowerToAir::crossesInterference): Deleted.
+ * b3/testb3.cpp:
+ (JSC::B3::testLoadBaseIndexShift2):
+ (JSC::B3::testLoadBaseIndexShift32):
+ (JSC::B3::run):
+
</ins><span class="cx"> 2016-10-11 Saam Barati <sbarati@apple.com>
</span><span class="cx">
</span><span class="cx"> ValueAdd should be constant folded if the operands are constant String,Primitive or Primitive,String
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3LowerToAircpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp (207162 => 207163)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2016-10-11 20:37:51 UTC (rev 207163)
</span><span class="lines">@@ -425,6 +425,28 @@
</span><span class="cx"> ASSERT_NOT_REACHED();
</span><span class="cx"> return true;
</span><span class="cx"> }
</span><ins>+
+ Optional<unsigned> scaleForShl(Value* shl, int32_t offset, Optional<Arg::Width> width = Nullopt)
+ {
+ if (shl->opcode() != Shl)
+ return Nullopt;
+ if (!shl->child(1)->hasInt32())
+ return Nullopt;
+ unsigned logScale = shl->child(1)->asInt32();
+ if (shl->type() == Int32)
+ logScale &= 31;
+ else
+ logScale &= 63;
+ // Use 64-bit math to perform the shift so that <<32 does the right thing, but then switch
+ // to signed since that's what all of our APIs want.
+ int64_t bigScale = static_cast<uint64_t>(1) << static_cast<uint64_t>(logScale);
+ if (!isRepresentableAs<int32_t>(bigScale))
+ return Nullopt;
+ unsigned scale = static_cast<int32_t>(bigScale);
+ if (!Arg::isValidIndexForm(scale, offset, width))
+ return Nullopt;
+ return scale;
+ }
</ins><span class="cx">
</span><span class="cx"> // This turns the given operand into an address.
</span><span class="cx"> Arg effectiveAddr(Value* address, int32_t offset, Arg::Width width)
</span><span class="lines">@@ -447,18 +469,12 @@
</span><span class="cx"> Value* right = address->child(1);
</span><span class="cx">
</span><span class="cx"> auto tryIndex = [&] (Value* index, Value* base) -> Arg {
</span><del>- if (index->opcode() != Shl)
</del><ins>+ Optional<unsigned> scale = scaleForShl(index, offset, width);
+ if (!scale)
</ins><span class="cx"> return Arg();
</span><span class="cx"> if (m_locked.contains(index->child(0)) || m_locked.contains(base))
</span><span class="cx"> return Arg();
</span><del>- if (!index->child(1)->hasInt32())
- return Arg();
-
- unsigned scale = 1 << (index->child(1)->asInt32() & 31);
- if (!Arg::isValidIndexForm(scale, offset, width))
- return Arg();
-
- return Arg::index(tmp(base), tmp(index->child(0)), scale, offset);
</del><ins>+ return Arg::index(tmp(base), tmp(index->child(0)), *scale, offset);
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> if (Arg result = tryIndex(left, right))
</span><span class="lines">@@ -1898,29 +1914,16 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> auto tryShl = [&] (Value* shl, Value* other) -> bool {
</span><del>- if (shl->opcode() != Shl)
</del><ins>+ Optional<unsigned> scale = scaleForShl(shl, offset);
+ if (!scale)
</ins><span class="cx"> return false;
</span><span class="cx"> if (!canBeInternal(shl))
</span><span class="cx"> return false;
</span><del>- if (!shl->child(1)->hasInt32())
- return false;
- unsigned logScale = shl->child(1)->asInt32();
- if (m_value->type() == Int32)
- logScale &= 31;
- else
- logScale &= 63;
- // Use 64-bit math to perform the shift so that <<32 does the right thing.
- int64_t bigScale = static_cast<uint64_t>(1) << static_cast<uint64_t>(logScale);
- if (!isRepresentableAs<int32_t>(bigScale))
- return false;
- unsigned scale = static_cast<int32_t>(bigScale);
- if (!Arg::isValidIndexForm(scale, offset))
- return false;
</del><span class="cx">
</span><span class="cx"> ASSERT(!m_locked.contains(shl->child(0)));
</span><span class="cx"> ASSERT(!m_locked.contains(other));
</span><span class="cx">
</span><del>- append(leaOpcode, Arg::index(tmp(other), tmp(shl->child(0)), scale, offset), tmp(m_value));
</del><ins>+ append(leaOpcode, Arg::index(tmp(other), tmp(shl->child(0)), *scale, offset), tmp(m_value));
</ins><span class="cx"> commitInternal(innerAdd);
</span><span class="cx"> commitInternal(shl);
</span><span class="cx"> return true;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3testb3cpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/testb3.cpp (207162 => 207163)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/testb3.cpp 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/JavaScriptCore/b3/testb3.cpp 2016-10-11 20:37:51 UTC (rev 207163)
</span><span class="lines">@@ -13659,6 +13659,52 @@
</span><span class="cx"> (root->last()->child(0)->child(0)->child(0) == arg2 && root->last()->child(0)->child(0)->child(1) == arg1));
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void testLoadBaseIndexShift2()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ root->appendNew<Value>(
+ proc, Return, Origin(),
+ root->appendNew<MemoryValue>(
+ proc, Load, Int32, Origin(),
+ root->appendNew<Value>(
+ proc, Add, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0),
+ root->appendNew<Value>(
+ proc, Shl, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1),
+ root->appendNew<Const32Value>(proc, Origin(), 2)))));
+ auto code = compile(proc);
+ if (isX86())
+ checkUsesInstruction(*code, "(%rdi,%rsi,4)");
+ int32_t value = 12341234;
+ char* ptr = bitwise_cast<char*>(&value);
+ for (unsigned i = 0; i < 10; ++i)
+ CHECK_EQ(invoke<int32_t>(*code, ptr - (static_cast<intptr_t>(1) << static_cast<intptr_t>(2)) * i, i), 12341234);
+}
+
+void testLoadBaseIndexShift32()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ root->appendNew<Value>(
+ proc, Return, Origin(),
+ root->appendNew<MemoryValue>(
+ proc, Load, Int32, Origin(),
+ root->appendNew<Value>(
+ proc, Add, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0),
+ root->appendNew<Value>(
+ proc, Shl, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1),
+ root->appendNew<Const32Value>(proc, Origin(), 32)))));
+ auto code = compile(proc);
+ int32_t value = 12341234;
+ char* ptr = bitwise_cast<char*>(&value);
+ for (unsigned i = 0; i < 10; ++i)
+ CHECK_EQ(invoke<int32_t>(*code, ptr - (static_cast<intptr_t>(1) << static_cast<intptr_t>(32)) * i, i), 12341234);
+}
+
</ins><span class="cx"> // Make sure the compiler does not try to optimize anything out.
</span><span class="cx"> NEVER_INLINE double zero()
</span><span class="cx"> {
</span><span class="lines">@@ -15095,6 +15141,8 @@
</span><span class="cx"> RUN(testAddShl32());
</span><span class="cx"> RUN(testAddShl64());
</span><span class="cx"> RUN(testAddShl65());
</span><ins>+ RUN(testLoadBaseIndexShift2());
+ RUN(testLoadBaseIndexShift32());
</ins><span class="cx">
</span><span class="cx"> if (isX86()) {
</span><span class="cx"> RUN(testBranchBitAndImmFusion(Identity, Int64, 1, Air::BranchTest32, Air::Arg::Tmp));
</span></span></pre>
</div>
</div>
</body>
</html>