<!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>[199337] 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/199337">199337</a></dd>
<dt>Author</dt> <dd>commit-queue@webkit.org</dd>
<dt>Date</dt> <dd>2016-04-11 23:16:21 -0700 (Mon, 11 Apr 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>[JSC] B3 can use undefined bits or not defined required bits when spilling
https://bugs.webkit.org/show_bug.cgi?id=156486
Patch by Benjamin Poulain <bpoulain@apple.com> on 2016-04-11
Reviewed by Filip Pizlo.
Spilling had issues when replacing arguments in place.
The problems are:
1) If we have a 32bit stackslot, a x86 instruction could still try to load 64bits from it.
2) If we have a 64bit stackslot, Move32 would only set half the bits.
3) We were reducing Move to Move32 even if the top bits are read from the stack slot.
The case 1 appear with something like this:
Move32 %tmp0, %tmp1
Op64 %tmp1, %tmp2, %tmp3
When we spill %tmp1, the stack slot is 32bit, Move32 sets 32bits
but Op64 supports addressing for %tmp1. When we substitute %tmp1 in Op64,
we are creating a 64bit read for a 32bit stack slot.
The case 2 is an other common one. If we have:
BB#1
Move32 %tmp0, %tmp1
Jump #3
BB#2
Op64 %tmp0, %tmp1
Jump #3
BB#3
Use64 %tmp1
We have a stack slot of 64bits. When spilling %tmp1 in #1, we are
effectively doing a 32bit store on the stack slot, leaving the top bits undefined.
Case 3 is pretty much the same as 2 but we create the Move32 ourself
because the source is a 32bit with ZDef.
Case (1) is solved by requiring that the stack slot is at least as large as the largest
use/def of that tmp.
Case (2) and (3) are solved by not replacing a Tmp by an Address if the Def
is smaller than the stack slot.
* b3/air/AirIteratedRegisterCoalescing.cpp:
* b3/testb3.cpp:
(JSC::B3::testSpillDefSmallerThanUse):
(JSC::B3::testSpillUseLargerThanDef):
(JSC::B3::run):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airAirIteratedRegisterCoalescingcpp">trunk/Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airAirTmpWidthh">trunk/Source/JavaScriptCore/b3/air/AirTmpWidth.h</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 (199336 => 199337)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-04-12 06:12:49 UTC (rev 199336)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-04-12 06:16:21 UTC (rev 199337)
</span><span class="lines">@@ -1,3 +1,52 @@
</span><ins>+2016-04-11 Benjamin Poulain <bpoulain@apple.com>
+
+ [JSC] B3 can use undefined bits or not defined required bits when spilling
+ https://bugs.webkit.org/show_bug.cgi?id=156486
+
+ Reviewed by Filip Pizlo.
+
+ Spilling had issues when replacing arguments in place.
+
+ The problems are:
+ 1) If we have a 32bit stackslot, a x86 instruction could still try to load 64bits from it.
+ 2) If we have a 64bit stackslot, Move32 would only set half the bits.
+ 3) We were reducing Move to Move32 even if the top bits are read from the stack slot.
+
+ The case 1 appear with something like this:
+ Move32 %tmp0, %tmp1
+ Op64 %tmp1, %tmp2, %tmp3
+ When we spill %tmp1, the stack slot is 32bit, Move32 sets 32bits
+ but Op64 supports addressing for %tmp1. When we substitute %tmp1 in Op64,
+ we are creating a 64bit read for a 32bit stack slot.
+
+ The case 2 is an other common one. If we have:
+ BB#1
+ Move32 %tmp0, %tmp1
+ Jump #3
+ BB#2
+ Op64 %tmp0, %tmp1
+ Jump #3
+ BB#3
+ Use64 %tmp1
+
+ We have a stack slot of 64bits. When spilling %tmp1 in #1, we are
+ effectively doing a 32bit store on the stack slot, leaving the top bits undefined.
+
+ Case 3 is pretty much the same as 2 but we create the Move32 ourself
+ because the source is a 32bit with ZDef.
+
+ Case (1) is solved by requiring that the stack slot is at least as large as the largest
+ use/def of that tmp.
+
+ Case (2) and (3) are solved by not replacing a Tmp by an Address if the Def
+ is smaller than the stack slot.
+
+ * b3/air/AirIteratedRegisterCoalescing.cpp:
+ * b3/testb3.cpp:
+ (JSC::B3::testSpillDefSmallerThanUse):
+ (JSC::B3::testSpillUseLargerThanDef):
+ (JSC::B3::run):
+
</ins><span class="cx"> 2016-04-11 Brian Burg <bburg@apple.com>
</span><span class="cx">
</span><span class="cx"> Web Inspector: get rid of InspectorBasicValue and InspectorString subclasses
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airAirIteratedRegisterCoalescingcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp (199336 => 199337)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp 2016-04-12 06:12:49 UTC (rev 199336)
+++ trunk/Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp 2016-04-12 06:16:21 UTC (rev 199337)
</span><span class="lines">@@ -1419,6 +1419,11 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ static unsigned stackSlotMinimumWidth(Arg::Width width)
+ {
+ return width <= Arg::Width32 ? 4 : 8;
+ }
+
</ins><span class="cx"> template<Arg::Type type>
</span><span class="cx"> void addSpillAndFill(const ColoringAllocator<type>& allocator, HashSet<unsigned>& unspillableTmps)
</span><span class="cx"> {
</span><span class="lines">@@ -1429,7 +1434,7 @@
</span><span class="cx">
</span><span class="cx"> // Allocate stack slot for each spilled value.
</span><span class="cx"> StackSlot* stackSlot = m_code.addStackSlot(
</span><del>- m_tmpWidth.width(tmp) <= Arg::Width32 ? 4 : 8, StackSlotKind::Spill);
</del><ins>+ stackSlotMinimumWidth(m_tmpWidth.requiredWidth(tmp)), StackSlotKind::Spill);
</ins><span class="cx"> bool isNewTmp = stackSlots.add(tmp, stackSlot).isNewEntry;
</span><span class="cx"> ASSERT_UNUSED(isNewTmp, isNewTmp);
</span><span class="cx"> }
</span><span class="lines">@@ -1447,30 +1452,39 @@
</span><span class="cx"> // only claim to read 32 bits from the source if only 32 bits of the destination are
</span><span class="cx"> // read. Note that we only apply this logic if this turns into a load or store, since
</span><span class="cx"> // Move is the canonical way to move data between GPRs.
</span><del>- bool forceMove32IfDidSpill = false;
</del><ins>+ bool canUseMove32IfDidSpill = false;
</ins><span class="cx"> bool didSpill = false;
</span><span class="cx"> if (type == Arg::GP && inst.opcode == Move) {
</span><span class="cx"> if ((inst.args[0].isTmp() && m_tmpWidth.width(inst.args[0].tmp()) <= Arg::Width32)
</span><span class="cx"> || (inst.args[1].isTmp() && m_tmpWidth.width(inst.args[1].tmp()) <= Arg::Width32))
</span><del>- forceMove32IfDidSpill = true;
</del><ins>+ canUseMove32IfDidSpill = true;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Try to replace the register use by memory use when possible.
</span><span class="cx"> inst.forEachArg(
</span><del>- [&] (Arg& arg, Arg::Role, Arg::Type argType, Arg::Width width) {
</del><ins>+ [&] (Arg& arg, Arg::Role role, Arg::Type argType, Arg::Width width) {
</ins><span class="cx"> if (arg.isTmp() && argType == type && !arg.isReg()) {
</span><span class="cx"> auto stackSlotEntry = stackSlots.find(arg.tmp());
</span><span class="cx"> if (stackSlotEntry != stackSlots.end()
</span><span class="cx"> && inst.admitsStack(arg)) {
</span><ins>+
+ Arg::Width spillWidth = m_tmpWidth.requiredWidth(arg.tmp());
+ if (Arg::isAnyDef(role) && width < spillWidth)
+ return;
+ ASSERT(inst.opcode == Move || !(Arg::isAnyUse(role) && width > spillWidth));
+
+ if (spillWidth != Arg::Width32)
+ canUseMove32IfDidSpill = false;
+
</ins><span class="cx"> stackSlotEntry->value->ensureSize(
</span><del>- forceMove32IfDidSpill ? 4 : Arg::bytes(width));
</del><ins>+ canUseMove32IfDidSpill ? 4 : Arg::bytes(width));
</ins><span class="cx"> arg = Arg::stack(stackSlotEntry->value);
</span><span class="cx"> didSpill = true;
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx"> });
</span><span class="cx">
</span><del>- if (didSpill && forceMove32IfDidSpill)
</del><ins>+ if (didSpill && canUseMove32IfDidSpill)
</ins><span class="cx"> inst.opcode = Move32;
</span><span class="cx">
</span><span class="cx"> // For every other case, add Load/Store as needed.
</span><span class="lines">@@ -1488,9 +1502,9 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- Arg arg = Arg::stack(stackSlotEntry->value);
</del><ins>+ Arg::Width spillWidth = m_tmpWidth.requiredWidth(tmp);
</ins><span class="cx"> Opcode move = Oops;
</span><del>- switch (stackSlotEntry->value->byteSize()) {
</del><ins>+ switch (stackSlotMinimumWidth(spillWidth)) {
</ins><span class="cx"> case 4:
</span><span class="cx"> move = type == Arg::GP ? Move32 : MoveFloat;
</span><span class="cx"> break;
</span><span class="lines">@@ -1505,6 +1519,7 @@
</span><span class="cx"> tmp = m_code.newTmp(type);
</span><span class="cx"> unspillableTmps.add(AbsoluteTmpMapper<type>::absoluteIndex(tmp));
</span><span class="cx">
</span><ins>+ Arg arg = Arg::stack(stackSlotEntry->value);
</ins><span class="cx"> if (Arg::isAnyUse(role) && role != Arg::Scratch)
</span><span class="cx"> insertionSet.insert(instIndex, move, inst.origin, arg, tmp);
</span><span class="cx"> if (Arg::isAnyDef(role))
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airAirTmpWidthh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/AirTmpWidth.h (199336 => 199337)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/AirTmpWidth.h 2016-04-12 06:12:49 UTC (rev 199336)
+++ trunk/Source/JavaScriptCore/b3/air/AirTmpWidth.h 2016-04-12 06:16:21 UTC (rev 199337)
</span><span class="lines">@@ -60,6 +60,15 @@
</span><span class="cx"> return std::min(iter->value.use, iter->value.def);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ // Return the minimum required width for all defs/uses of this Tmp.
+ Arg::Width requiredWidth(Tmp tmp)
+ {
+ auto iter = m_width.find(tmp);
+ if (iter == m_width.end())
+ return Arg::minimumWidth(Arg(tmp).type());
+ return std::max(iter->value.use, iter->value.def);
+ }
+
</ins><span class="cx"> // This indirectly tells you how much of the tmp's high bits are guaranteed to be zero. The number of
</span><span class="cx"> // high bits that are zero are:
</span><span class="cx"> //
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3testb3cpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/testb3.cpp (199336 => 199337)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/testb3.cpp 2016-04-12 06:12:49 UTC (rev 199336)
+++ trunk/Source/JavaScriptCore/b3/testb3.cpp 2016-04-12 06:16:21 UTC (rev 199337)
</span><span class="lines">@@ -11408,6 +11408,97 @@
</span><span class="cx"> CHECK(invoke<double>(*code, 42.5) == 42.5);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void testSpillDefSmallerThanUse()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+
+ // Move32.
+ Value* arg32 = root->appendNew<Value>(
+ proc, Trunc, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
+ Value* arg64 = root->appendNew<Value>(proc, ZExt32, Origin(), arg32);
+
+ // Make sure arg64 is on the stack.
+ PatchpointValue* forceSpill = root->appendNew<PatchpointValue>(proc, Int64, Origin());
+ RegisterSet clobberSet = RegisterSet::allGPRs();
+ clobberSet.exclude(RegisterSet::stackRegisters());
+ clobberSet.exclude(RegisterSet::reservedHardwareRegisters());
+ clobberSet.clear(GPRInfo::returnValueGPR); // Force the return value for aliasing below.
+ forceSpill->clobberLate(clobberSet);
+ forceSpill->setGenerator(
+ [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+ AllowMacroScratchRegisterUsage allowScratch(jit);
+ jit.xor64(params[0].gpr(), params[0].gpr());
+ });
+
+ // On x86, Sub admit an address for any operand. If it uses the stack, the top bits must be zero.
+ Value* result = root->appendNew<Value>(proc, Sub, Origin(), forceSpill, arg64);
+ root->appendNew<ControlValue>(proc, Return, Origin(), result);
+
+ auto code = compile(proc);
+ CHECK(invoke<int64_t>(*code, 0xffffffff00000000) == 0);
+}
+
+void testSpillUseLargerThanDef()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ BasicBlock* thenCase = proc.addBlock();
+ BasicBlock* elseCase = proc.addBlock();
+ BasicBlock* tail = proc.addBlock();
+
+ RegisterSet clobberSet = RegisterSet::allGPRs();
+ clobberSet.exclude(RegisterSet::stackRegisters());
+ clobberSet.exclude(RegisterSet::reservedHardwareRegisters());
+
+ Value* condition = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+ Value* argument = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1);
+ root->appendNew<ControlValue>(
+ proc, Branch, Origin(),
+ root->appendNew<Value>(
+ proc, Trunc, Origin(),
+ condition),
+ FrequentedBlock(thenCase), FrequentedBlock(elseCase));
+
+ Value* truncated = thenCase->appendNew<Value>(proc, ZExt32, Origin(),
+ thenCase->appendNew<Value>(proc, Trunc, Origin(), argument));
+ UpsilonValue* thenResult = thenCase->appendNew<UpsilonValue>(proc, Origin(), truncated);
+ thenCase->appendNew<ControlValue>(proc, Jump, Origin(), FrequentedBlock(tail));
+
+ UpsilonValue* elseResult = elseCase->appendNew<UpsilonValue>(proc, Origin(), argument);
+ elseCase->appendNew<ControlValue>(proc, Jump, Origin(), FrequentedBlock(tail));
+
+ for (unsigned i = 0; i < 100; ++i) {
+ PatchpointValue* preventTailDuplication = tail->appendNew<PatchpointValue>(proc, Void, Origin());
+ preventTailDuplication->clobberLate(clobberSet);
+ preventTailDuplication->setGenerator([] (CCallHelpers&, const StackmapGenerationParams&) { });
+ }
+
+ PatchpointValue* forceSpill = tail->appendNew<PatchpointValue>(proc, Void, Origin());
+ forceSpill->clobberLate(clobberSet);
+ forceSpill->setGenerator(
+ [&] (CCallHelpers& jit, const StackmapGenerationParams&) {
+ AllowMacroScratchRegisterUsage allowScratch(jit);
+ clobberSet.forEach([&] (Reg reg) {
+ jit.move(CCallHelpers::TrustedImm64(0xffffffffffffffff), reg.gpr());
+ });
+ });
+
+ Value* phi = tail->appendNew<Value>(proc, Phi, Int64, Origin());
+ thenResult->setPhi(phi);
+ elseResult->setPhi(phi);
+ tail->appendNew<ControlValue>(proc, Return, Origin(), phi);
+
+ auto code = compile(proc);
+ CHECK(invoke<uint64_t>(*code, 1, 0xffffffff00000000) == 0);
+ CHECK(invoke<uint64_t>(*code, 0, 0xffffffff00000000) == 0xffffffff00000000);
+
+ // A second time since the previous run is still on the stack.
+ CHECK(invoke<uint64_t>(*code, 1, 0xffffffff00000000) == 0);
+
+}
+
</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">@@ -12784,6 +12875,9 @@
</span><span class="cx">
</span><span class="cx"> RUN(testPatchpointDoubleRegs());
</span><span class="cx">
</span><ins>+ RUN(testSpillDefSmallerThanUse());
+ RUN(testSpillUseLargerThanDef());
+
</ins><span class="cx"> if (tasks.isEmpty())
</span><span class="cx"> usage();
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>