- Revision
- 183406
- Author
- [email protected]
- Date
- 2015-04-27 11:49:15 -0700 (Mon, 27 Apr 2015)
Log Message
VarargsForwardingPhase should use bytecode liveness in addition to other uses to determine the last point that a candidate is used
https://bugs.webkit.org/show_bug.cgi?id=143843
Reviewed by Geoffrey Garen.
It will soon come to pass that Phantom isn't available at the time that
VarargsForwardingPhase runs. So, it needs to use some other mechanism for discovering when
a value dies for OSR.
This is simplified by two things:
1) The bytecode kill analysis is now reusable. This patch makes it even more reusable than
before by polishing the API.
2) This phase already operates on one node at a time and allows itself to do a full search
of the enclosing basic block for that node. This is fine because CreateDirectArguments
and friends is a rarely occurring node. The fact that it operates on one node at a time
makes it even easier to reason about OSR liveness - we just track the list of locals in
which it is live.
This change has no effect right now but it is a necessary prerequisite to implementing
https://bugs.webkit.org/show_bug.cgi?id=143736.
* dfg/DFGBasicBlock.h:
(JSC::DFG::BasicBlock::tryAt):
* dfg/DFGForAllKills.h:
(JSC::DFG::forAllKilledOperands):
* dfg/DFGPhantomInsertionPhase.cpp:
* dfg/DFGVarargsForwardingPhase.cpp:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (183405 => 183406)
--- trunk/Source/_javascript_Core/ChangeLog 2015-04-27 18:46:15 UTC (rev 183405)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-04-27 18:49:15 UTC (rev 183406)
@@ -1,3 +1,35 @@
+2015-04-25 Filip Pizlo <[email protected]>
+
+ VarargsForwardingPhase should use bytecode liveness in addition to other uses to determine the last point that a candidate is used
+ https://bugs.webkit.org/show_bug.cgi?id=143843
+
+ Reviewed by Geoffrey Garen.
+
+ It will soon come to pass that Phantom isn't available at the time that
+ VarargsForwardingPhase runs. So, it needs to use some other mechanism for discovering when
+ a value dies for OSR.
+
+ This is simplified by two things:
+
+ 1) The bytecode kill analysis is now reusable. This patch makes it even more reusable than
+ before by polishing the API.
+
+ 2) This phase already operates on one node at a time and allows itself to do a full search
+ of the enclosing basic block for that node. This is fine because CreateDirectArguments
+ and friends is a rarely occurring node. The fact that it operates on one node at a time
+ makes it even easier to reason about OSR liveness - we just track the list of locals in
+ which it is live.
+
+ This change has no effect right now but it is a necessary prerequisite to implementing
+ https://bugs.webkit.org/show_bug.cgi?id=143736.
+
+ * dfg/DFGBasicBlock.h:
+ (JSC::DFG::BasicBlock::tryAt):
+ * dfg/DFGForAllKills.h:
+ (JSC::DFG::forAllKilledOperands):
+ * dfg/DFGPhantomInsertionPhase.cpp:
+ * dfg/DFGVarargsForwardingPhase.cpp:
+
2015-04-27 Jordan Harband <[email protected]>
Map#entries and Map#keys error for non-Maps is swapped
Modified: trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h (183405 => 183406)
--- trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h 2015-04-27 18:46:15 UTC (rev 183405)
+++ trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h 2015-04-27 18:49:15 UTC (rev 183406)
@@ -61,6 +61,12 @@
bool isEmpty() const { return !size(); }
Node*& at(size_t i) { return m_nodes[i]; }
Node* at(size_t i) const { return m_nodes[i]; }
+ Node* tryAt(size_t i) const
+ {
+ if (i >= size())
+ return nullptr;
+ return at(i);
+ }
Node*& operator[](size_t i) { return at(i); }
Node* operator[](size_t i) const { return at(i); }
Modified: trunk/Source/_javascript_Core/dfg/DFGForAllKills.h (183405 => 183406)
--- trunk/Source/_javascript_Core/dfg/DFGForAllKills.h 2015-04-27 18:46:15 UTC (rev 183405)
+++ trunk/Source/_javascript_Core/dfg/DFGForAllKills.h 2015-04-27 18:49:15 UTC (rev 183406)
@@ -75,6 +75,12 @@
void forAllKilledOperands(Graph& graph, Node* nodeBefore, Node* nodeAfter, const Functor& functor)
{
CodeOrigin before = nodeBefore->origin.forExit;
+
+ if (!nodeAfter) {
+ graph.forAllLiveInBytecode(before, functor);
+ return;
+ }
+
CodeOrigin after = nodeAfter->origin.forExit;
VirtualRegister alreadyNoted;
Modified: trunk/Source/_javascript_Core/dfg/DFGPhantomInsertionPhase.cpp (183405 => 183406)
--- trunk/Source/_javascript_Core/dfg/DFGPhantomInsertionPhase.cpp 2015-04-27 18:46:15 UTC (rev 183405)
+++ trunk/Source/_javascript_Core/dfg/DFGPhantomInsertionPhase.cpp 2015-04-27 18:49:15 UTC (rev 183406)
@@ -122,42 +122,35 @@
node->setEpoch(currentEpoch);
- auto killAction = [&] (VirtualRegister reg) {
- if (verbose)
- dataLog(" Killed operand: ", reg, "\n");
-
- Node* killedNode = m_values.operand(reg);
- if (!killedNode)
- return;
-
- // We only need to insert a Phantom if the node hasn't been used since the last
- // exit, and was born before the last exit.
- if (killedNode->epoch() == currentEpoch)
- return;
-
- if (verbose) {
- dataLog(
- " Inserting Phantom on ", killedNode, " after ",
- block->at(lastExitingIndex), "\n");
- }
-
- // We have exact ref counts, so creating a new use means that we have to increment
- // the ref count.
- killedNode->postfixRef();
-
- m_insertionSet.insertNode(
- lastExitingIndex + 1, SpecNone, Phantom, block->at(lastExitingIndex)->origin,
- killedNode->defaultEdge());
- };
-
- if (nodeIndex + 1 == block->size()) {
- // Should a MovHinted value be kept alive? If the value has been SetLocal'd then
- // the answer is no. But we may have a value that is live here and dead in
- // successors because we had jettisoned those successors that would have used the
- // value. Hence, anything live here should be kept alive.
- m_graph.forAllLiveInBytecode(node->origin.forExit, killAction);
- } else
- forAllKilledOperands(m_graph, node, block->at(nodeIndex + 1), killAction);
+ forAllKilledOperands(
+ m_graph, node, block->tryAt(nodeIndex + 1),
+ [&] (VirtualRegister reg) {
+ if (verbose)
+ dataLog(" Killed operand: ", reg, "\n");
+
+ Node* killedNode = m_values.operand(reg);
+ if (!killedNode)
+ return;
+
+ // We only need to insert a Phantom if the node hasn't been used since the last
+ // exit, and was born before the last exit.
+ if (killedNode->epoch() == currentEpoch)
+ return;
+
+ if (verbose) {
+ dataLog(
+ " Inserting Phantom on ", killedNode, " after ",
+ block->at(lastExitingIndex), "\n");
+ }
+
+ // We have exact ref counts, so creating a new use means that we have to
+ // increment the ref count.
+ killedNode->postfixRef();
+
+ m_insertionSet.insertNode(
+ lastExitingIndex + 1, SpecNone, Phantom,
+ block->at(lastExitingIndex)->origin, killedNode->defaultEdge());
+ });
}
m_insertionSet.execute(block);
Modified: trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp (183405 => 183406)
--- trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp 2015-04-27 18:46:15 UTC (rev 183405)
+++ trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp 2015-04-27 18:49:15 UTC (rev 183406)
@@ -30,6 +30,7 @@
#include "DFGArgumentsUtilities.h"
#include "DFGClobberize.h"
+#include "DFGForAllKills.h"
#include "DFGGraph.h"
#include "DFGPhase.h"
#include "JSCInlines.h"
@@ -49,6 +50,8 @@
bool run()
{
+ DFG_ASSERT(m_graph, nullptr, m_graph.m_form != SSA);
+
if (verbose) {
dataLog("Graph before varargs forwarding:\n");
m_graph.dump();
@@ -87,13 +90,19 @@
// Find the index of the last node in this block to use the candidate, and look for escaping
// sites.
unsigned lastUserIndex = candidateNodeIndex;
+ Vector<VirtualRegister, 2> relevantLocals; // This is a set. We expect it to be a small set.
for (unsigned nodeIndex = candidateNodeIndex + 1; nodeIndex < block->size(); ++nodeIndex) {
Node* node = block->at(nodeIndex);
+
switch (node->op()) {
+ case MovHint:
+ lastUserIndex = nodeIndex;
+ if (!relevantLocals.contains(node->unlinkedLocal()))
+ relevantLocals.append(node->unlinkedLocal());
+ break;
+
case Phantom:
case Check:
- case MovHint:
- case PutHint:
case LoadVarargs:
if (m_graph.uses(node, candidate))
lastUserIndex = nodeIndex;
@@ -125,6 +134,18 @@
return;
}
}
+
+ forAllKilledOperands(
+ m_graph, node, block->tryAt(nodeIndex + 1),
+ [&] (VirtualRegister reg) {
+ for (unsigned i = 0; i < relevantLocals.size(); ++i) {
+ if (relevantLocals[i] == reg) {
+ relevantLocals[i--] = relevantLocals.last();
+ relevantLocals.removeLast();
+ lastUserIndex = nodeIndex;
+ }
+ }
+ });
}
if (verbose)
dataLog("Selected lastUserIndex = ", lastUserIndex, ", ", block->at(lastUserIndex), "\n");
