<!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>[194628] 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/194628">194628</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-01-05 19:43:40 -0800 (Tue, 05 Jan 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>FTL B3 fails cdjs-tests.yaml/red_black_tree_test.js.ftl-eager-no-cjit
https://bugs.webkit.org/show_bug.cgi?id=152770
Reviewed by Mark Lam.
It turns out that liveness didn't know that the return value GPR or FPR is live at the
return. Consequently, we can end up with code that clobbers the return value register after
the move of the return value into that register. This could happen if we start with
something like:
Move 42(%tmp1), %tmp2
Move 50(%tmp1), %tmp3
Move %tmp3, 58(%tmp1)
Move %tmp2, %rax
Ret
Then we might coalesce %tmp2 with %rax:
Move 42(%tmp1), %rax
Move 50(%tmp1), %tmp3
Move %tmp3, 58(%tmp1)
Ret
But now there is no use of %rax after that first instruction, so %rax appears dead at the
other two Move's. So, the register allocator could then do this:
Move 42(%tmp1), %rax
Move 50(%tmp1), %rax
Move %rax, 58(%tmp1)
Ret
And that's clearly wrong. This patch solves this issue by replacing the old Ret instruction
with Ret32, Ret64, RetFloat, and RetDouble. These all take the return value register as an
argument. They also tell Air which parts of the return value register the caller will
observe. That's great for width analysis.
This resolves a test failure in the CDjs red_black_tree_test. This reduces the total number
of JSC test failures from 217 to 191.
* assembler/MacroAssembler.h:
(JSC::MacroAssembler::oops):
(JSC::MacroAssembler::ret32):
(JSC::MacroAssembler::ret64):
(JSC::MacroAssembler::retFloat):
(JSC::MacroAssembler::retDouble):
(JSC::MacroAssembler::shouldConsiderBlinding):
* b3/B3LowerToAir.cpp:
(JSC::B3::Air::LowerToAir::lower):
* b3/air/AirGenerate.cpp:
(JSC::B3::Air::generate):
* b3/air/AirHandleCalleeSaves.cpp:
(JSC::B3::Air::handleCalleeSaves):
* b3/air/AirOpcode.opcodes:
* b3/air/opcode_generator.rb:</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreassemblerMacroAssemblerh">trunk/Source/JavaScriptCore/assembler/MacroAssembler.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3B3LowerToAircpp">trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airAirGeneratecpp">trunk/Source/JavaScriptCore/b3/air/AirGenerate.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airAirHandleCalleeSavescpp">trunk/Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airAirOpcodeopcodes">trunk/Source/JavaScriptCore/b3/air/AirOpcode.opcodes</a></li>
<li><a href="#trunkSourceJavaScriptCoreb3airopcode_generatorrb">trunk/Source/JavaScriptCore/b3/air/opcode_generator.rb</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -1,3 +1,60 @@
</span><ins>+2016-01-05 Filip Pizlo <fpizlo@apple.com>
+
+ FTL B3 fails cdjs-tests.yaml/red_black_tree_test.js.ftl-eager-no-cjit
+ https://bugs.webkit.org/show_bug.cgi?id=152770
+
+ Reviewed by Mark Lam.
+
+ It turns out that liveness didn't know that the return value GPR or FPR is live at the
+ return. Consequently, we can end up with code that clobbers the return value register after
+ the move of the return value into that register. This could happen if we start with
+ something like:
+
+ Move 42(%tmp1), %tmp2
+ Move 50(%tmp1), %tmp3
+ Move %tmp3, 58(%tmp1)
+ Move %tmp2, %rax
+ Ret
+
+ Then we might coalesce %tmp2 with %rax:
+
+ Move 42(%tmp1), %rax
+ Move 50(%tmp1), %tmp3
+ Move %tmp3, 58(%tmp1)
+ Ret
+
+ But now there is no use of %rax after that first instruction, so %rax appears dead at the
+ other two Move's. So, the register allocator could then do this:
+
+ Move 42(%tmp1), %rax
+ Move 50(%tmp1), %rax
+ Move %rax, 58(%tmp1)
+ Ret
+
+ And that's clearly wrong. This patch solves this issue by replacing the old Ret instruction
+ with Ret32, Ret64, RetFloat, and RetDouble. These all take the return value register as an
+ argument. They also tell Air which parts of the return value register the caller will
+ observe. That's great for width analysis.
+
+ This resolves a test failure in the CDjs red_black_tree_test. This reduces the total number
+ of JSC test failures from 217 to 191.
+
+ * assembler/MacroAssembler.h:
+ (JSC::MacroAssembler::oops):
+ (JSC::MacroAssembler::ret32):
+ (JSC::MacroAssembler::ret64):
+ (JSC::MacroAssembler::retFloat):
+ (JSC::MacroAssembler::retDouble):
+ (JSC::MacroAssembler::shouldConsiderBlinding):
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::lower):
+ * b3/air/AirGenerate.cpp:
+ (JSC::B3::Air::generate):
+ * b3/air/AirHandleCalleeSaves.cpp:
+ (JSC::B3::Air::handleCalleeSaves):
+ * b3/air/AirOpcode.opcodes:
+ * b3/air/opcode_generator.rb:
+
</ins><span class="cx"> 2016-01-05 Keith Miller <keith_miller@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed build fix. A symbol was being exported that should not have been.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreassemblerMacroAssemblerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/assembler/MacroAssembler.h (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/assembler/MacroAssembler.h 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/assembler/MacroAssembler.h 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -477,6 +477,13 @@
</span><span class="cx"> abortWithReason(B3Oops);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ // B3 has additional pseudo-opcodes for returning, when it wants to signal that the return
+ // consumes some register in some way.
+ void ret32(RegisterID) { ret(); }
+ void ret64(RegisterID) { ret(); }
+ void retFloat(FPRegisterID) { ret(); }
+ void retDouble(FPRegisterID) { ret(); }
+
</ins><span class="cx"> static const unsigned BlindingModulus = 64;
</span><span class="cx"> bool shouldConsiderBlinding()
</span><span class="cx"> {
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3B3LowerToAircpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -2175,17 +2175,31 @@
</span><span class="cx">
</span><span class="cx"> case Return: {
</span><span class="cx"> Value* value = m_value->child(0);
</span><del>- Air::Opcode move;
- Tmp dest;
- if (isInt(value->type())) {
- move = Move;
- dest = Tmp(GPRInfo::returnValueGPR);
- } else {
- move = MoveDouble;
- dest = Tmp(FPRInfo::returnValueFPR);
</del><ins>+ Tmp returnValueGPR = Tmp(GPRInfo::returnValueGPR);
+ Tmp returnValueFPR = Tmp(FPRInfo::returnValueFPR);
+ switch (value->type()) {
+ case Void:
+ // It's impossible for a void value to be used as a child. If we did want to have a
+ // void return, we'd introduce a different opcode, like ReturnVoid.
+ RELEASE_ASSERT_NOT_REACHED();
+ break;
+ case Int32:
+ append(Move, immOrTmp(value), returnValueGPR);
+ append(Ret32, returnValueGPR);
+ break;
+ case Int64:
+ append(Move, immOrTmp(value), returnValueGPR);
+ append(Ret64, returnValueGPR);
+ break;
+ case Float:
+ append(MoveFloat, tmp(value), returnValueFPR);
+ append(RetFloat, returnValueFPR);
+ break;
+ case Double:
+ append(MoveDouble, tmp(value), returnValueFPR);
+ append(RetDouble, returnValueFPR);
+ break;
</ins><span class="cx"> }
</span><del>- append(move, immOrTmp(value), dest);
- append(Ret);
</del><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airAirGeneratecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/AirGenerate.cpp (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/AirGenerate.cpp 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/b3/air/AirGenerate.cpp 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -35,6 +35,7 @@
</span><span class="cx"> #include "AirGenerationContext.h"
</span><span class="cx"> #include "AirHandleCalleeSaves.h"
</span><span class="cx"> #include "AirIteratedRegisterCoalescing.h"
</span><ins>+#include "AirOpcodeUtils.h"
</ins><span class="cx"> #include "AirOptimizeBlockOrder.h"
</span><span class="cx"> #include "AirReportUsedRegisters.h"
</span><span class="cx"> #include "AirSimplifyCFG.h"
</span><span class="lines">@@ -152,7 +153,7 @@
</span><span class="cx"> && block->successorBlock(0) == code.findNextBlock(block))
</span><span class="cx"> continue;
</span><span class="cx">
</span><del>- if (block->last().opcode == Ret) {
</del><ins>+ if (isReturn(block->last().opcode)) {
</ins><span class="cx"> // We currently don't represent the full prologue/epilogue in Air, so we need to
</span><span class="cx"> // have this override.
</span><span class="cx"> if (code.frameSize())
</span><span class="lines">@@ -162,7 +163,7 @@
</span><span class="cx"> jit.ret();
</span><span class="cx"> continue;
</span><span class="cx"> }
</span><del>-
</del><ins>+
</ins><span class="cx"> CCallHelpers::Jump jump = block->last().generate(jit, context);
</span><span class="cx"> switch (block->numSuccessors()) {
</span><span class="cx"> case 0:
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airAirHandleCalleeSavescpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -89,7 +89,7 @@
</span><span class="cx"> // Now insert restore code at epilogues.
</span><span class="cx"> for (BasicBlock* block : code) {
</span><span class="cx"> Inst& last = block->last();
</span><del>- if (last.opcode != Ret)
</del><ins>+ if (!isReturn(last.opcode))
</ins><span class="cx"> continue;
</span><span class="cx">
</span><span class="cx"> for (const RegisterAtOffset& entry : code.calleeSaveRegisters()) {
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airAirOpcodeopcodes"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/AirOpcode.opcodes (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/AirOpcode.opcodes 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/b3/air/AirOpcode.opcodes 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -634,8 +634,18 @@
</span><span class="cx">
</span><span class="cx"> Jump /branch
</span><span class="cx">
</span><del>-Ret /terminal
</del><ins>+Ret32 U:G:32 /return
+ Tmp
</ins><span class="cx">
</span><ins>+64: Ret64 U:G:64 /return
+ Tmp
+
+RetFloat U:F:32 /return
+ Tmp
+
+RetDouble U:F:64 /return
+ Tmp
+
</ins><span class="cx"> Oops /terminal
</span><span class="cx">
</span><span class="cx"> # Air allows for exotic behavior. A Patch's behavior is determined entirely by the Special operand,
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreb3airopcode_generatorrb"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/b3/air/opcode_generator.rb (194627 => 194628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/b3/air/opcode_generator.rb 2016-01-06 03:27:32 UTC (rev 194627)
+++ trunk/Source/JavaScriptCore/b3/air/opcode_generator.rb 2016-01-06 03:43:40 UTC (rev 194628)
</span><span class="lines">@@ -385,10 +385,14 @@
</span><span class="cx"> case token.string
</span><span class="cx"> when "branch"
</span><span class="cx"> opcode.attributes[:branch] = true
</span><ins>+ opcode.attributes[:terminal] = true
</ins><span class="cx"> when "terminal"
</span><span class="cx"> opcode.attributes[:terminal] = true
</span><span class="cx"> when "effects"
</span><span class="cx"> opcode.attributes[:effects] = true
</span><ins>+ when "return"
+ opcode.attributes[:return] = true
+ opcode.attributes[:terminal] = true
</ins><span class="cx"> else
</span><span class="cx"> parseError("Bad / directive")
</span><span class="cx"> end
</span><span class="lines">@@ -698,17 +702,40 @@
</span><span class="cx"> outp.puts "inline bool isTerminal(Opcode opcode)"
</span><span class="cx"> outp.puts "{"
</span><span class="cx"> outp.puts "switch (opcode) {"
</span><ins>+ didFindTerminals = false
</ins><span class="cx"> $opcodes.values.each {
</span><span class="cx"> | opcode |
</span><del>- if opcode.attributes[:terminal] or opcode.attributes[:branch]
</del><ins>+ if opcode.attributes[:terminal]
</ins><span class="cx"> outp.puts "case #{opcode.name}:"
</span><ins>+ didFindTerminals = true
</ins><span class="cx"> end
</span><span class="cx"> }
</span><del>- outp.puts "return true;"
</del><ins>+ if didFindTerminals
+ outp.puts "return true;"
+ end
</ins><span class="cx"> outp.puts "default:"
</span><span class="cx"> outp.puts "return false;"
</span><span class="cx"> outp.puts "}"
</span><span class="cx"> outp.puts "}"
</span><ins>+
+ outp.puts "inline bool isReturn(Opcode opcode)"
+ outp.puts "{"
+ outp.puts "switch (opcode) {"
+ didFindReturns = false
+ $opcodes.values.each {
+ | opcode |
+ if opcode.attributes[:return]
+ outp.puts "case #{opcode.name}:"
+ didFindReturns = true
+ end
+ }
+ if didFindReturns
+ outp.puts "return true;"
+ end
+ outp.puts "default:"
+ outp.puts "return false;"
+ outp.puts "}"
+ outp.puts "}"
</ins><span class="cx">
</span><span class="cx"> outp.puts "} } } // namespace JSC::B3::Air"
</span><span class="cx"> }
</span><span class="lines">@@ -960,7 +987,7 @@
</span><span class="cx"> foundTrue = false
</span><span class="cx"> $opcodes.values.each {
</span><span class="cx"> | opcode |
</span><del>- if opcode.attributes[:branch] or opcode.attributes[:terminal] or opcode.attributes[:effects]
</del><ins>+ if opcode.attributes[:terminal] or opcode.attributes[:effects]
</ins><span class="cx"> outp.puts "case #{opcode.name}:"
</span><span class="cx"> foundTrue = true
</span><span class="cx"> end
</span></span></pre>
</div>
</div>
</body>
</html>