Diff
Modified: tags/Safari-601.1.12.3/Source/_javascript_Core/ChangeLog (177456 => 177457)
--- tags/Safari-601.1.12.3/Source/_javascript_Core/ChangeLog 2014-12-17 21:03:53 UTC (rev 177456)
+++ tags/Safari-601.1.12.3/Source/_javascript_Core/ChangeLog 2014-12-17 21:08:28 UTC (rev 177457)
@@ -1,3 +1,26 @@
+2014-12-17 Matthew Hanson <matthew_han...@apple.com>
+
+ Merge r177270. rdar://problem/19245579
+
+ 2014-12-14 Filip Pizlo <fpi...@apple.com>
+
+ PutLocalSinkingPhase has an invalid assertion about incoming values, because both liveness and deferral analyses are conservative
+ https://bugs.webkit.org/show_bug.cgi?id=139630
+
+ Reviewed by Oliver Hunt.
+
+ Replaces a faulty assertion with code to handle an awesome special case. Also adds a lot of
+ comments that reconstruct my reasoning about this code. I had to work hard to remember how
+ deferral worked so I wrote my discoveries down.
+
+ * dfg/DFGInsertionSet.h:
+ (JSC::DFG::InsertionSet::insertBottomConstantForUse):
+ * dfg/DFGPutLocalSinkingPhase.cpp:
+ * tests/stress/put-local-conservative.js: Added.
+ (foo):
+ (.result):
+ (bar):
+
2014-12-11 Babak Shafiei <bshaf...@apple.com>
Merge r177210.
Modified: tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGInsertionSet.h (177456 => 177457)
--- tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGInsertionSet.h 2014-12-17 21:03:53 UTC (rev 177456)
+++ tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGInsertionSet.h 2014-12-17 21:08:28 UTC (rev 177457)
@@ -115,7 +115,16 @@
{
return insertConstantForUse(index, NodeOrigin(origin), value, useKind);
}
-
+
+ Edge insertBottomConstantForUse(size_t index, NodeOrigin origin, UseKind useKind)
+ {
+ if (isDouble(useKind))
+ return insertConstantForUse(index, origin, jsNumber(PNaN), useKind);
+ if (useKind == Int52RepUse)
+ return insertConstantForUse(index, origin, jsNumber(0), useKind);
+ return insertConstantForUse(index, origin, jsUndefined(), useKind);
+ }
+
Node* insertOutOfOrder(const Insertion& insertion)
{
size_t targetIndex = insertion.index();
Modified: tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGPutLocalSinkingPhase.cpp (177456 => 177457)
--- tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGPutLocalSinkingPhase.cpp 2014-12-17 21:03:53 UTC (rev 177456)
+++ tags/Safari-601.1.12.3/Source/_javascript_Core/dfg/DFGPutLocalSinkingPhase.cpp 2014-12-17 21:08:28 UTC (rev 177457)
@@ -214,7 +214,50 @@
// Next identify where we would want to sink PutLocals to. We say that there is a deferred
// flush if we had a PutLocal with a given VariableAccessData* but it hasn't been
- // materialized yet.
+ // materialized yet. Deferrals have the following lattice; but it's worth noting that the
+ // TOP part of the lattice serves an entirely different purpose than the rest of the lattice:
+ // it just means that we're in a region of code where nobody should have been relying on the
+ // value. The rest of the lattice means that we either have a PutLocal that is deferred (i.e.
+ // still needs to be executed) or there isn't one (because we've alraedy executed it).
+ //
+ // Bottom:
+ // Instantiated as VariableDeferral().
+ // Means that all previous PutLocals have been executed so there is nothing deferred.
+ // During merging this is subordinate to the other kinds of deferrals, because it
+ // represents the fact that we've already executed all necessary PutLocals. This implies
+ // that there *had* been some PutLocals that we should have executed.
+ //
+ // Top:
+ // Instantiated as VariableDeferral::conflict().
+ // Represents the fact that we know, via forward flow, that there isn't any value in the
+ // given local that anyone should have been relying on. This comes into play at the
+ // prologue (because in SSA form at the prologue no local has any value) or when we merge
+ // deferrals for different VariableAccessData*'s. A VAD encompasses a lexical scope in
+ // which the local has some semantic meaning; if we had stores from different lexical
+ // scopes that got merged together then we know that we're not in either scope anymore.
+ // Note that this is all approximate and only precise enough to later answer questions
+ // pertinent to sinking. For example, this doesn't always detect when a local is no
+ // longer semantically relevant - we may well have a deferral from inside some inlined
+ // call survive outside of that inlined code, and this is generally OK. In the worst case
+ // it means that we might think that a deferral that is actually dead must still be
+ // executed. But we usually catch that with liveness. Liveness doesn't always catch it
+ // because liveness is conservative.
+ //
+ // What Top does give us is detects situations where we both don't need to care about a
+ // deferral and there is no way that we could reason about it anyway. If we merged
+ // deferrals for different variables then we wouldn't know the format to use. So, we
+ // use Top in that case because that's also a case where we know that we can ignore the
+ // deferral.
+ //
+ // Deferral with a concrete VariableAccessData*:
+ // Instantiated as VariableDeferral(someVariableAccessData)
+ // Represents the fact that the original code would have done a PutLocal but we haven't
+ // identified an operation that would have observed that PutLocal.
+ //
+ // This code has some interesting quirks because of the fact that neither liveness nor
+ // deferrals are very precise. They are only precise enough to be able to correctly tell us
+ // when we may [sic] need to execute PutLocals. This means that they may report the need to
+ // execute a PutLocal in cases where we actually don't really need it, and that's totally OK.
BlockMap<Operands<VariableDeferral>> deferredAtHead(m_graph);
BlockMap<Operands<VariableDeferral>> deferredAtTail(m_graph);
@@ -465,7 +508,34 @@
FlushFormat format = variableDeferral.variable()->flushFormat();
UseKind useKind = useKindFor(format);
Node* incoming = mapping.operand(operand);
- DFG_ASSERT(m_graph, nullptr, incoming);
+ if (!incoming) {
+ // This can totally happen, see tests/stress/put-local-conservative.js.
+ // This arises because deferral and liveness are both conservative.
+ // Conservative liveness means that a load from a *different* closure
+ // variable may lead us to believe that our local is live. Conservative
+ // deferral may lead us to believe that the local doesn't have a top deferral
+ // because someone has done something that would have forced it to be
+ // materialized. The basic pattern is:
+ //
+ // GetClosureVar(loc42) // loc42's deferral is now bottom
+ // if (predicate1)
+ // PutClosureVar(loc42) // prevent GCSE of our GetClosureVar's
+ // if (predicate2)
+ // PutLocal(loc42) // we now have a concrete deferral
+ // // we still have the concrete deferral because we merged with bottom
+ // GetClosureVar(loc42) // force materialization
+ //
+ // We will have a Phi with no incoming value form the basic block that
+ // bypassed the PutLocal.
+
+ // Note: we sort of could have used the equivalent of LLVM's undef here. The
+ // point is that it's OK to just leave random bits in the local if we're
+ // coming down this path. But, we don't have a way of saying that in our IR
+ // right now and anyway it probably doesn't matter that much.
+
+ incoming = insertionSet.insertBottomConstantForUse(
+ upsilonInsertionPoint, upsilonOrigin, useKind).node();
+ }
insertionSet.insertNode(
upsilonInsertionPoint, SpecNone, Upsilon, upsilonOrigin,
Added: tags/Safari-601.1.12.3/Source/_javascript_Core/tests/stress/put-local-conservative.js (0 => 177457)
--- tags/Safari-601.1.12.3/Source/_javascript_Core/tests/stress/put-local-conservative.js (rev 0)
+++ tags/Safari-601.1.12.3/Source/_javascript_Core/tests/stress/put-local-conservative.js 2014-12-17 21:08:28 UTC (rev 177457)
@@ -0,0 +1,47 @@
+function foo(o, a, b, c) {
+ // Don't do anything real but have some control flow. This causes the PutLocals for a,
+ // b, and c to survive into SSA form. But we don't have any effects, so sinking will be
+ // successful.
+ if (o.f)
+ return 42;
+ else
+ return 0;
+}
+
+function bar(o, y) {
+ var a = y;
+ var b = y + 1;
+ var c = y + 2;
+ var d = y + 3;
+ var e = y + 4;
+ var f = y + 5;
+ var g = y + 6;
+ var h = y + 7;
+ var i = y + 8;
+ var j = y + 9;
+ var k = y + 10;
+ var result = function(p, q) {
+ var x = a + b + c + d + e + f + g + h + i + j + k;
+ if (q) {
+ // Make it appear that it's possible to clobber those closure variables, so that we
+ // load from them again down below.
+ a = b = c = d = e = f = g = h = i = j = k = 42;
+ }
+ if (p)
+ x = foo(o, 1, 2, 3)
+ else
+ x = 5;
+ return x + a + b + c + d + e + f + g + h + i + j + k;
+ };
+ noInline(result);
+ return result;
+}
+
+var o = {f: 42};
+
+for (var i = 0; i < 100000; ++i) {
+ var result = bar(o, i)(true, false);
+ if (result != 42 + 11 * i + 55)
+ throw "Error: bad result: " + result;
+}
+