Modified: trunk/Source/_javascript_Core/ChangeLog (192189 => 192190)
--- trunk/Source/_javascript_Core/ChangeLog 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-11-10 00:39:13 UTC (rev 192190)
@@ -1,3 +1,32 @@
+2015-11-09 Saam barati <sbar...@apple.com>
+
+ DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
+ https://bugs.webkit.org/show_bug.cgi?id=145295
+
+ Reviewed by Filip Pizlo.
+
+ This patch fixes PutStackSinkingPhase to no longer escape the stack
+ locations that LoadVarargs and ForwardVarargs write to. We used
+ to consider sinking PutStacks right before a LoadVarargs/ForwardVarargs
+ because we considered them uses of such stack locations. They aren't
+ uses of those stack locations, they unconditionally write to those
+ stack locations. Sinking PutStacks to these nodes was not needed before,
+ but seemed mostly innocent. But I ran into a problem with this while implementing
+ FTL try/catch where we would end up having to generate a value for a sunken PutStack
+ right before a LoadVarargs. This would cause us to issue a GetStack that loaded garbage that
+ was then forwarded into a Phi that was used as the source as the PutStack. This caused the
+ abstract interpreter to confuse itself on type information for the garbage GetStack
+ that was fed into the Phi, which would cause the abstract interpreter to then claim
+ that the basic block with the PutStack in it would never be reached. This isn't true, the
+ block would indeed be reached. The solution here is to be more precise about the
+ liveness of locals w.r.t LoadVarargs and ForwardVarargs.
+
+ * dfg/DFGPreciseLocalClobberize.h:
+ (JSC::DFG::PreciseLocalClobberizeAdaptor::PreciseLocalClobberizeAdaptor):
+ (JSC::DFG::PreciseLocalClobberizeAdaptor::write):
+ * dfg/DFGPutStackSinkingPhase.cpp:
+ * dfg/DFGSSACalculator.h:
+
2015-11-09 Filip Pizlo <fpi...@apple.com>
B3->Air lowering should support CCall
Modified: trunk/Source/_javascript_Core/dfg/DFGPreciseLocalClobberize.h (192189 => 192190)
--- trunk/Source/_javascript_Core/dfg/DFGPreciseLocalClobberize.h 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/_javascript_Core/dfg/DFGPreciseLocalClobberize.h 2015-11-10 00:39:13 UTC (rev 192190)
@@ -42,7 +42,7 @@
: m_graph(graph)
, m_node(node)
, m_read(read)
- , m_write(write)
+ , m_unconditionalWrite(write)
, m_def(def)
{
}
@@ -70,7 +70,7 @@
// We expect stack writes to already be precisely characterized by DFG::clobberize().
if (heap.kind() == Stack) {
RELEASE_ASSERT(!heap.payload().isTop());
- callIfAppropriate(m_write, VirtualRegister(heap.payload().value32()));
+ callIfAppropriate(m_unconditionalWrite, VirtualRegister(heap.payload().value32()));
return;
}
@@ -155,7 +155,7 @@
Graph& m_graph;
Node* m_node;
const ReadFunctor& m_read;
- const WriteFunctor& m_write;
+ const WriteFunctor& m_unconditionalWrite;
const DefFunctor& m_def;
};
Modified: trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp (192189 => 192190)
--- trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2015-11-10 00:39:13 UTC (rev 192190)
@@ -108,31 +108,29 @@
if (verbose)
dataLog("Live at ", node, ": ", live, "\n");
+ Vector<VirtualRegister, 4> reads;
+ Vector<VirtualRegister, 4> writes;
auto escapeHandler = [&] (VirtualRegister operand) {
if (operand.isHeader())
return;
if (verbose)
dataLog(" ", operand, " is live at ", node, "\n");
- live.operand(operand) = true;
+ reads.append(operand);
};
-
- // FIXME: This might mishandle LoadVarargs and ForwardVarargs. It might make us
- // think that the locals being written are stack-live here. They aren't. This
- // should be harmless since we overwrite them anyway, but still, it's sloppy.
- // https://bugs.webkit.org/show_bug.cgi?id=145295
+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ writes.append(operand);
+ };
+
preciseLocalClobberize(
- m_graph, node, escapeHandler, escapeHandler,
- [&] (VirtualRegister operand, LazyNode source) {
- RELEASE_ASSERT(source.isNode());
+ m_graph, node, escapeHandler, writeHandler,
+ [&] (VirtualRegister, LazyNode) { });
- if (source.asNode() == node) {
- // This is a load. Ignore it.
- return;
- }
-
- RELEASE_ASSERT(node->op() == PutStack);
- live.operand(operand) = false;
- });
+ for (VirtualRegister operand : writes)
+ live.operand(operand) = false;
+ for (VirtualRegister operand : reads)
+ live.operand(operand) = true;
}
if (live == liveAtHead[block])
@@ -205,10 +203,13 @@
// Represents the fact that the original code would have done a PutStack but we haven't
// identified an operation that would have observed that PutStack.
//
- // 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 PutStacks. This means that they may report the need to
- // execute a PutStack in cases where we actually don't really need it, and that's totally OK.
+ // We need to be precise about liveness in this phase because not doing so
+ // could cause us to insert a PutStack before a node we thought may escape a
+ // value that it doesn't really escape. Sinking this PutStack above such a node may
+ // cause us to insert a GetStack that we forward to the Phi we're feeding into the
+ // sunken PutStack. Inserting such a GetStack could cause us to load garbage and
+ // can confuse the AI to claim untrue things (like that the program will exit when
+ // it really won't).
BlockMap<Operands<FlushFormat>> deferredAtHead(m_graph);
BlockMap<Operands<FlushFormat>> deferredAtTail(m_graph);
@@ -269,6 +270,10 @@
// A GetStack doesn't affect anything, since we know which local we are reading
// from.
continue;
+ } else if (node->op() == PutStack) {
+ VirtualRegister operand = node->stackAccessData()->local;
+ deferred.operand(operand) = node->stackAccessData()->format;
+ continue;
}
auto escapeHandler = [&] (VirtualRegister operand) {
@@ -279,19 +284,15 @@
// We will materialize just before any reads.
deferred.operand(operand) = DeadFlush;
};
+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ deferred.operand(operand) = DeadFlush;
+ };
preciseLocalClobberize(
- m_graph, node, escapeHandler, escapeHandler,
- [&] (VirtualRegister operand, LazyNode source) {
- RELEASE_ASSERT(source.isNode());
-
- if (source.asNode() == node) {
- // This is a load. Ignore it.
- return;
- }
-
- deferred.operand(operand) = node->stackAccessData()->format;
- });
+ m_graph, node, escapeHandler, writeHandler,
+ [&] (VirtualRegister, LazyNode) { });
}
if (deferred == deferredAtTail[block])
@@ -351,13 +352,13 @@
indexToOperand.append(operand);
}
- HashSet<Node*> putLocalsToSink;
+ HashSet<Node*> putStacksToSink;
for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
for (Node* node : *block) {
switch (node->op()) {
case PutStack:
- putLocalsToSink.add(node);
+ putStacksToSink.add(node);
ssaCalculator.newDef(
operandToVariable.operand(node->stackAccessData()->local),
block, node->child1().node());
@@ -496,9 +497,19 @@
deferred.operand(operand) = DeadFlush;
};
-
+
+ auto writeHandler = [&] (VirtualRegister operand) {
+ // LoadVarargs and ForwardVarargs are unconditional writes to the stack
+ // locations they claim to write to. They do not read from the stack
+ // locations they write to. This makes those stack locations dead right
+ // before a LoadVarargs/ForwardVarargs. This means we should never sink
+ // PutStacks right to this point.
+ RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
+ deferred.operand(operand) = DeadFlush;
+ };
+
preciseLocalClobberize(
- m_graph, node, escapeHandler, escapeHandler,
+ m_graph, node, escapeHandler, writeHandler,
[&] (VirtualRegister, LazyNode) { });
break;
} }
@@ -552,13 +563,11 @@
for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
Node* node = block->at(nodeIndex);
- if (!putLocalsToSink.contains(node))
+ if (!putStacksToSink.contains(node))
continue;
node->remove();
}
-
- insertionSet.execute(block);
}
if (verbose) {
Modified: trunk/Source/_javascript_Core/dfg/DFGSSACalculator.h (192189 => 192190)
--- trunk/Source/_javascript_Core/dfg/DFGSSACalculator.h 2015-11-10 00:36:14 UTC (rev 192189)
+++ trunk/Source/_javascript_Core/dfg/DFGSSACalculator.h 2015-11-10 00:39:13 UTC (rev 192190)
@@ -91,10 +91,10 @@
// FIXME: Make it easier to do this, that doesn't involve rerunning GCSE.
// https://bugs.webkit.org/show_bug.cgi?id=136639
//
-// 4.3) Insert Upsilons for each Phi in each successor block. Use the available values table to
-// decide the source value for each Phi's variable. Note that you could also use
-// SSACalculator::reachingDefAtTail() instead of the available values table, though your
-// local available values table is likely to be more efficient.
+// 4.3) Insert Upsilons at the end of the current block for the corresponding Phis in each successor block.
+// Use the available values table to decide the source value for each Phi's variable. Note that
+// you could also use SSACalculator::reachingDefAtTail() instead of the available values table,
+// though your local available values table is likely to be more efficient.
//
// The most obvious use of SSACalculator is for the CPS->SSA conversion itself, but it's meant to
// also be used for SSA update and for things like the promotion of heap fields to local SSA
