<!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>[192377] 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/192377">192377</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-11-12 12:41:06 -0800 (Thu, 12 Nov 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>B3 should be able to compile Patchpoints with Register and Any constraints
https://bugs.webkit.org/show_bug.cgi?id=151209
Reviewed by Geoffrey Garen.
Most of this patch is about adding tests, since Register constraints already worked but
were untested, and almost all forms of the Any constraint worked.
One change that I'm making is that Patchpoints now default to forCall effects rather
than no effects. I think this is better because if a client of Patchpoint never supplies
any effects, it's best to assume the worst.
My testing uncovered only one real bug: moveConstants() would turn all non-zero double
constants into Loads, so the optimization to fold constants into a Stackmap would stop
working. Instead of telling the Stackmap that the double value it wanted is actually a
constant, we would tell it that it's a register and it would emit code to materialize
the double into that register. I worked around this by having moveConstants() create
ConstDoubleValue's just for the Stackmaps, which lowerToAir() recognizes and does the
right thing with (i.e. folds into the stackmap instead of materializing).
* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::fillStackmap):
(JSC::B3::Air::LowerToAir::lower):
* b3/B3MoveConstants.cpp:
* b3/B3PatchpointValue.cpp:
(JSC::B3::PatchpointValue::PatchpointValue):
* b3/testb3.cpp:
(JSC::B3::testPatchpointCallArg):
(JSC::B3::testPatchpointFixedRegister):
(JSC::B3::testPatchpointAny):
(JSC::B3::testPatchpointAnyImm):
(JSC::B3::testPatchpointManyImms):
(JSC::B3::testSimpleCheck):
(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="#trunkSourceJavaScriptCoreb3B3MoveConstantscpp">trunk/Source/JavaScriptCore/b3/B3MoveConstants.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3PatchpointValuecpp">trunk/Source/JavaScriptCore/b3/B3PatchpointValue.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 (192376 => 192377)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-11-12 20:06:41 UTC (rev 192376)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-11-12 20:41:06 UTC (rev 192377)
</span><span class="lines">@@ -1,3 +1,40 @@
</span><ins>+2015-11-12 Filip Pizlo <fpizlo@apple.com>
+
+ B3 should be able to compile Patchpoints with Register and Any constraints
+ https://bugs.webkit.org/show_bug.cgi?id=151209
+
+ Reviewed by Geoffrey Garen.
+
+ Most of this patch is about adding tests, since Register constraints already worked but
+ were untested, and almost all forms of the Any constraint worked.
+
+ One change that I'm making is that Patchpoints now default to forCall effects rather
+ than no effects. I think this is better because if a client of Patchpoint never supplies
+ any effects, it's best to assume the worst.
+
+ My testing uncovered only one real bug: moveConstants() would turn all non-zero double
+ constants into Loads, so the optimization to fold constants into a Stackmap would stop
+ working. Instead of telling the Stackmap that the double value it wanted is actually a
+ constant, we would tell it that it's a register and it would emit code to materialize
+ the double into that register. I worked around this by having moveConstants() create
+ ConstDoubleValue's just for the Stackmaps, which lowerToAir() recognizes and does the
+ right thing with (i.e. folds into the stackmap instead of materializing).
+
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::fillStackmap):
+ (JSC::B3::Air::LowerToAir::lower):
+ * b3/B3MoveConstants.cpp:
+ * b3/B3PatchpointValue.cpp:
+ (JSC::B3::PatchpointValue::PatchpointValue):
+ * b3/testb3.cpp:
+ (JSC::B3::testPatchpointCallArg):
+ (JSC::B3::testPatchpointFixedRegister):
+ (JSC::B3::testPatchpointAny):
+ (JSC::B3::testPatchpointAnyImm):
+ (JSC::B3::testPatchpointManyImms):
+ (JSC::B3::testSimpleCheck):
+ (JSC::B3::run):
+
</ins><span class="cx"> 2015-11-12 Mark Lam <mark.lam@apple.com>
</span><span class="cx">
</span><span class="cx"> Move some prototypes to header files.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3LowerToAircpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp (192376 => 192377)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2015-11-12 20:06:41 UTC (rev 192376)
+++ trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2015-11-12 20:41:06 UTC (rev 192377)
</span><span class="lines">@@ -732,9 +732,10 @@
</span><span class="cx"> arg = imm(value.value());
</span><span class="cx"> else if (value.value()->hasInt64())
</span><span class="cx"> arg = Arg::imm64(value.value()->asInt64());
</span><del>- else if (value.value()->hasDouble())
</del><ins>+ else if (value.value()->hasDouble() && canBeInternal(value.value())) {
+ commitInternal(value.value());
</ins><span class="cx"> arg = Arg::imm64(bitwise_cast<int64_t>(value.value()->asDouble()));
</span><del>- else
</del><ins>+ } else
</ins><span class="cx"> arg = tmp(value.value());
</span><span class="cx"> break;
</span><span class="cx"> case ValueRep::SomeRegister:
</span><span class="lines">@@ -1383,7 +1384,8 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> case ConstDouble: {
</span><del>- // We expect that the moveConstants() phase has run.
</del><ins>+ // We expect that the moveConstants() phase has run, and any doubles referenced from
+ // stackmaps get fused.
</ins><span class="cx"> RELEASE_ASSERT(isIdentical(m_value->asDouble(), 0.0));
</span><span class="cx"> append(MoveZeroToDouble, tmp(m_value));
</span><span class="cx"> return;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3MoveConstantscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3MoveConstants.cpp (192376 => 192377)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3MoveConstants.cpp 2015-11-12 20:06:41 UTC (rev 192376)
+++ trunk/Source/JavaScriptCore/b3/B3MoveConstants.cpp 2015-11-12 20:41:06 UTC (rev 192377)
</span><span class="lines">@@ -82,11 +82,28 @@
</span><span class="cx">
</span><span class="cx"> for (unsigned valueIndex = 0; valueIndex < block->size(); ++valueIndex) {
</span><span class="cx"> Value* value = block->at(valueIndex);
</span><del>- for (Value*& child : value->children()) {
</del><ins>+ StackmapValue* stackmap = value->as<StackmapValue>();
+ for (unsigned childIndex = 0; childIndex < value->numChildren(); ++childIndex) {
+ Value*& child = value->child(childIndex);
</ins><span class="cx"> if (!needsMotion(child))
</span><span class="cx"> continue;
</span><span class="cx">
</span><del>- child = materialize(valueIndex, child->key(), value->origin());
</del><ins>+ ValueKey key = child->key();
+ if (stackmap
+ && goesInTable(key)
+ && stackmap->constrainedChild(childIndex).rep() == ValueRep::Any) {
+ // This is a weird special case. When we constant-fold an argument to a
+ // stackmap, and that argument has the Any constraint, we want to just
+ // tell the stackmap's generator that the argument is a constant rather
+ // than materializing it in a register. For this to work, we need
+ // lowerToAir to see this argument as a constant rather than as a load
+ // from a table.
+ child = m_insertionSet.insertValue(
+ valueIndex, key.materialize(m_proc, value->origin()));
+ continue;
+ }
+
+ child = materialize(valueIndex, key, value->origin());
</ins><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3PatchpointValuecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3PatchpointValue.cpp (192376 => 192377)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3PatchpointValue.cpp 2015-11-12 20:06:41 UTC (rev 192376)
+++ trunk/Source/JavaScriptCore/b3/B3PatchpointValue.cpp 2015-11-12 20:41:06 UTC (rev 192377)
</span><span class="lines">@@ -36,6 +36,7 @@
</span><span class="cx">
</span><span class="cx"> PatchpointValue::PatchpointValue(unsigned index, Type type, Origin origin)
</span><span class="cx"> : StackmapValue(index, CheckedOpcode, Patchpoint, type, origin)
</span><ins>+ , effects(Effects::forCall())
</ins><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3testb3cpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/testb3.cpp (192376 => 192377)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/testb3.cpp 2015-11-12 20:06:41 UTC (rev 192376)
+++ trunk/Source/JavaScriptCore/b3/testb3.cpp 2015-11-12 20:41:06 UTC (rev 192377)
</span><span class="lines">@@ -2693,6 +2693,115 @@
</span><span class="cx"> CHECK(compileAndRun<int>(proc, 1, 2) == 3);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void testPatchpointFixedRegister()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ Value* arg1 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+ Value* arg2 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1);
+ PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+ patchpoint->append(ConstrainedValue(arg1, ValueRep(GPRInfo::regT0)));
+ patchpoint->append(ConstrainedValue(arg2, ValueRep(GPRInfo::regT1)));
+ patchpoint->setGenerator(
+ [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+ CHECK(params.reps.size() == 3);
+ CHECK(params.reps[0].isGPR());
+ CHECK(params.reps[1] == ValueRep(GPRInfo::regT0));
+ CHECK(params.reps[2] == ValueRep(GPRInfo::regT1));
+ GPRReg result = params.reps[0].gpr();
+
+ if (result == GPRInfo::regT1) {
+ jit.move(GPRInfo::regT1, result);
+ jit.add32(GPRInfo::regT0, result);
+ } else {
+ jit.move(GPRInfo::regT0, result);
+ jit.add32(GPRInfo::regT1, result);
+ }
+ });
+ root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+ CHECK(compileAndRun<int>(proc, 1, 2) == 3);
+}
+
+void testPatchpointAny()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ Value* arg1 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+ Value* arg2 = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1);
+ PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+ patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+ patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+ patchpoint->setGenerator(
+ [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+ // We shouldn't have spilled the inputs, so we assert that they're in registers.
+ CHECK(params.reps.size() == 3);
+ CHECK(params.reps[0].isGPR());
+ CHECK(params.reps[1].isGPR());
+ CHECK(params.reps[2].isGPR());
+ jit.move(params.reps[1].gpr(), params.reps[0].gpr());
+ jit.add32(params.reps[2].gpr(), params.reps[0].gpr());
+ });
+ root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+ CHECK(compileAndRun<int>(proc, 1, 2) == 3);
+}
+
+void testPatchpointAnyImm()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ Value* arg1 = root->appendNew<Value>(
+ proc, Trunc, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
+ Value* arg2 = root->appendNew<Const32Value>(proc, Origin(), 42);
+ PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Int32, Origin());
+ patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+ patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+ patchpoint->setGenerator(
+ [&] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+ CHECK(params.reps.size() == 3);
+ CHECK(params.reps[0].isGPR());
+ CHECK(params.reps[1].isGPR());
+ CHECK(params.reps[2].isConstant());
+ CHECK(params.reps[2].value() == 42);
+ jit.add32(
+ CCallHelpers::TrustedImm32(static_cast<int32_t>(params.reps[2].value())),
+ params.reps[1].gpr(), params.reps[0].gpr());
+ });
+ root->appendNew<ControlValue>(proc, Return, Origin(), patchpoint);
+
+ CHECK(compileAndRun<int>(proc, 1) == 43);
+}
+
+void testPatchpointManyImms()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ Value* arg1 = root->appendNew<Const32Value>(proc, Origin(), 42);
+ Value* arg2 = root->appendNew<Const64Value>(proc, Origin(), 43);
+ Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
+ Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
+ PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
+ patchpoint->append(ConstrainedValue(arg1, ValueRep::Any));
+ patchpoint->append(ConstrainedValue(arg2, ValueRep::Any));
+ patchpoint->append(ConstrainedValue(arg3, ValueRep::Any));
+ patchpoint->append(ConstrainedValue(arg4, ValueRep::Any));
+ patchpoint->setGenerator(
+ [&] (CCallHelpers&, const StackmapGenerationParams& params) {
+ CHECK(params.reps.size() == 4);
+ CHECK(params.reps[0] == ValueRep::constant(42));
+ CHECK(params.reps[1] == ValueRep::constant(43));
+ CHECK(params.reps[2] == ValueRep::constant(43000000000000ll));
+ CHECK(params.reps[3] == ValueRep::constant(bitwise_cast<int64_t>(42.5)));
+ });
+ root->appendNew<ControlValue>(
+ proc, Return, Origin(),
+ root->appendNew<Const32Value>(proc, Origin(), 0));
+
+ CHECK(!compileAndRun<int>(proc));
+}
+
</ins><span class="cx"> void testSimpleCheck()
</span><span class="cx"> {
</span><span class="cx"> Procedure proc;
</span><span class="lines">@@ -3841,6 +3950,10 @@
</span><span class="cx">
</span><span class="cx"> RUN(testSimplePatchpoint());
</span><span class="cx"> RUN(testPatchpointCallArg());
</span><ins>+ RUN(testPatchpointFixedRegister());
+ RUN(testPatchpointAny());
+ RUN(testPatchpointAnyImm());
+ RUN(testPatchpointManyImms());
</ins><span class="cx"> RUN(testSimpleCheck());
</span><span class="cx"> RUN(testCheckLessThan());
</span><span class="cx"> RUN(testCheckMegaCombo());
</span></span></pre>
</div>
</div>
</body>
</html>