- Revision
- 103218
- Author
- [email protected]
- Date
- 2011-12-18 22:36:05 -0800 (Sun, 18 Dec 2011)
Log Message
DFG is too sloppy with register allocation
https://bugs.webkit.org/show_bug.cgi?id=74835
Reviewed by Gavin Barraclough.
Added assertions that at the end of a successfully generated basic block,
all use counts should be zero. This revealed a number of bugs:
- Array length optimizations were turning a must-generate node into one
that is not must-generate, but failing to change the ref count
accordingly.
- Indexed property storage optimizations were failing to deref their
children, or to deref the indexed property storage node itself. Also,
they used the Phantom node as a replacement. But the Phantom node is
must-generate, which was causing bizarre issues. So this introduces a
Nop node, which should be used in cases where you want a node that is
skipped and has no children.
This does not have any significant performance effect, but it should
relieve some register pressure. The main thing this patch adds, though,
are the assertions, which should make it easier to do register allocation
related changes in the future.
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::execute):
* dfg/DFGGenerationInfo.h:
(JSC::DFG::GenerationInfo::initConstant):
(JSC::DFG::GenerationInfo::initInteger):
(JSC::DFG::GenerationInfo::initJSValue):
(JSC::DFG::GenerationInfo::initCell):
(JSC::DFG::GenerationInfo::initBoolean):
(JSC::DFG::GenerationInfo::initDouble):
(JSC::DFG::GenerationInfo::initStorage):
(JSC::DFG::GenerationInfo::use):
* dfg/DFGGraph.h:
(JSC::DFG::Graph::clearAndDerefChild1):
(JSC::DFG::Graph::clearAndDerefChild2):
(JSC::DFG::Graph::clearAndDerefChild3):
* dfg/DFGNode.h:
(JSC::DFG::Node::deref):
* dfg/DFGPropagator.cpp:
(JSC::DFG::Propagator::propagateNodePredictions):
(JSC::DFG::Propagator::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (103217 => 103218)
--- trunk/Source/_javascript_Core/ChangeLog 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/ChangeLog 2011-12-19 06:36:05 UTC (rev 103218)
@@ -1,3 +1,56 @@
+2011-12-18 Filip Pizlo <[email protected]>
+
+ DFG is too sloppy with register allocation
+ https://bugs.webkit.org/show_bug.cgi?id=74835
+
+ Reviewed by Gavin Barraclough.
+
+ Added assertions that at the end of a successfully generated basic block,
+ all use counts should be zero. This revealed a number of bugs:
+
+ - Array length optimizations were turning a must-generate node into one
+ that is not must-generate, but failing to change the ref count
+ accordingly.
+
+ - Indexed property storage optimizations were failing to deref their
+ children, or to deref the indexed property storage node itself. Also,
+ they used the Phantom node as a replacement. But the Phantom node is
+ must-generate, which was causing bizarre issues. So this introduces a
+ Nop node, which should be used in cases where you want a node that is
+ skipped and has no children.
+
+ This does not have any significant performance effect, but it should
+ relieve some register pressure. The main thing this patch adds, though,
+ are the assertions, which should make it easier to do register allocation
+ related changes in the future.
+
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::execute):
+ * dfg/DFGGenerationInfo.h:
+ (JSC::DFG::GenerationInfo::initConstant):
+ (JSC::DFG::GenerationInfo::initInteger):
+ (JSC::DFG::GenerationInfo::initJSValue):
+ (JSC::DFG::GenerationInfo::initCell):
+ (JSC::DFG::GenerationInfo::initBoolean):
+ (JSC::DFG::GenerationInfo::initDouble):
+ (JSC::DFG::GenerationInfo::initStorage):
+ (JSC::DFG::GenerationInfo::use):
+ * dfg/DFGGraph.h:
+ (JSC::DFG::Graph::clearAndDerefChild1):
+ (JSC::DFG::Graph::clearAndDerefChild2):
+ (JSC::DFG::Graph::clearAndDerefChild3):
+ * dfg/DFGNode.h:
+ (JSC::DFG::Node::deref):
+ * dfg/DFGPropagator.cpp:
+ (JSC::DFG::Propagator::propagateNodePredictions):
+ (JSC::DFG::Propagator::fixupNode):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+
2011-12-18 Benjamin Poulain <[email protected]>
Remove the duplicated code from ASCIICType.h
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2011-12-19 06:36:05 UTC (rev 103218)
@@ -892,6 +892,7 @@
case Phantom:
case InlineStart:
+ case Nop:
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGGenerationInfo.h (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGGenerationInfo.h 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGGenerationInfo.h 2011-12-19 06:36:05 UTC (rev 103218)
@@ -60,6 +60,7 @@
m_registerFormat = DataFormatNone;
m_spillFormat = DataFormatNone;
m_canFill = true;
+ ASSERT(m_useCount);
}
void initInteger(NodeIndex nodeIndex, uint32_t useCount, GPRReg gpr)
{
@@ -69,6 +70,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.gpr = gpr;
+ ASSERT(m_useCount);
}
#if USE(JSVALUE64)
void initJSValue(NodeIndex nodeIndex, uint32_t useCount, GPRReg gpr, DataFormat format = DataFormatJS)
@@ -81,6 +83,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.gpr = gpr;
+ ASSERT(m_useCount);
}
#elif USE(JSVALUE32_64)
void initJSValue(NodeIndex nodeIndex, uint32_t useCount, GPRReg tagGPR, GPRReg payloadGPR, DataFormat format = DataFormatJS)
@@ -94,6 +97,7 @@
m_canFill = false;
u.v.tagGPR = tagGPR;
u.v.payloadGPR = payloadGPR;
+ ASSERT(m_useCount);
}
#endif
void initCell(NodeIndex nodeIndex, uint32_t useCount, GPRReg gpr)
@@ -104,6 +108,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.gpr = gpr;
+ ASSERT(m_useCount);
}
void initBoolean(NodeIndex nodeIndex, uint32_t useCount, GPRReg gpr)
{
@@ -113,6 +118,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.gpr = gpr;
+ ASSERT(m_useCount);
}
void initDouble(NodeIndex nodeIndex, uint32_t useCount, FPRReg fpr)
{
@@ -123,6 +129,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.fpr = fpr;
+ ASSERT(m_useCount);
}
void initStorage(NodeIndex nodeIndex, uint32_t useCount, GPRReg gpr)
{
@@ -132,6 +139,7 @@
m_spillFormat = DataFormatNone;
m_canFill = false;
u.gpr = gpr;
+ ASSERT(m_useCount);
}
// Get the index of the node that produced this value.
@@ -142,6 +150,7 @@
// associated machine registers may be freed.
bool use()
{
+ ASSERT(m_useCount);
return !--m_useCount;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.h 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h 2011-12-19 06:36:05 UTC (rev 103218)
@@ -79,7 +79,31 @@
if (node.ref())
refChildren(nodeIndex);
}
+
+ void clearAndDerefChild1(Node& node)
+ {
+ if (node.children.fixed.child1 == NoNode)
+ return;
+ at(node.children.fixed.child1).deref();
+ node.children.fixed.child1 = NoNode;
+ }
+ void clearAndDerefChild2(Node& node)
+ {
+ if (node.children.fixed.child2 == NoNode)
+ return;
+ at(node.children.fixed.child2).deref();
+ node.children.fixed.child2 = NoNode;
+ }
+
+ void clearAndDerefChild3(Node& node)
+ {
+ if (node.children.fixed.child3 == NoNode)
+ return;
+ at(node.children.fixed.child3).deref();
+ node.children.fixed.child3 = NoNode;
+ }
+
#ifndef NDEBUG
// CodeBlock is optional, but may allow additional information to be dumped (e.g. Identifier names).
void dump(CodeBlock* = 0);
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGNode.h 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h 2011-12-19 06:36:05 UTC (rev 103218)
@@ -178,6 +178,7 @@
macro(GetLocal, NodeResultJS) \
macro(SetLocal, 0) \
macro(Phantom, NodeMustGenerate) \
+ macro(Nop, 0) \
macro(Phi, 0) \
macro(Flush, NodeMustGenerate) \
\
@@ -835,6 +836,11 @@
m_refCount = refCount;
}
+ void deref()
+ {
+ m_refCount--;
+ }
+
NodeIndex child1()
{
ASSERT(!(op & NodeHasVarArgs));
Modified: trunk/Source/_javascript_Core/dfg/DFGPropagator.cpp (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGPropagator.cpp 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGPropagator.cpp 2011-12-19 06:36:05 UTC (rev 103218)
@@ -624,6 +624,7 @@
// These gets ignored because it doesn't do anything.
case Phantom:
case InlineStart:
+ case Nop:
break;
#else
default:
@@ -915,22 +916,24 @@
node.op = GetFloat64ArrayLength;
else
ASSERT_NOT_REACHED();
+ node.deref(); // No longer MustGenerate
break;
}
case GetIndexedPropertyStorage: {
PredictedType basePrediction = m_graph[node.child2()].prediction();
if (!(basePrediction & PredictInt32) && basePrediction) {
- node.op = Phantom;
- node.children.fixed.child1 = NoNode;
- node.children.fixed.child2 = NoNode;
- node.children.fixed.child3 = NoNode;
+ node.op = Nop;
+ m_graph.clearAndDerefChild1(node);
+ m_graph.clearAndDerefChild2(node);
+ m_graph.clearAndDerefChild3(node);
+ node.setRefCount(0);
}
break;
}
case GetByVal:
case StringCharAt:
case StringCharCodeAt: {
- if (node.child3() != NoNode && m_graph[node.child3()].op == Phantom)
+ if (node.child3() != NoNode && m_graph[node.child3()].op == Nop)
node.children.fixed.child3 = NoNode;
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2011-12-19 06:36:05 UTC (rev 103218)
@@ -1042,6 +1042,14 @@
if (node.shouldGenerate())
checkConsistency();
}
+
+ // Perform the most basic verification that children have been used correctly.
+#if !ASSERT_DISABLED
+ for (unsigned index = 0; index < m_generationInfo.size(); ++index) {
+ GenerationInfo& info = m_generationInfo[index];
+ ASSERT(!info.alive());
+ }
+#endif
}
// If we are making type predictions about our arguments then
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2011-12-19 06:36:05 UTC (rev 103218)
@@ -3863,10 +3863,14 @@
break;
case InlineStart:
+ case Nop:
ASSERT_NOT_REACHED();
break;
}
+ if (!m_compileOkay)
+ return;
+
if (node.hasResult() && node.mustGenerate())
use(m_compileIndex);
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (103217 => 103218)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2011-12-19 06:26:46 UTC (rev 103217)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2011-12-19 06:36:05 UTC (rev 103218)
@@ -3796,10 +3796,14 @@
break;
case InlineStart:
+ case Nop:
ASSERT_NOT_REACHED();
break;
}
+ if (!m_compileOkay)
+ return;
+
if (node.hasResult() && node.mustGenerate())
use(m_compileIndex);
}
