Title: [118715] trunk/Source/_javascript_Core
- Revision
- 118715
- Author
- fpi...@apple.com
- Date
- 2012-05-28 19:43:30 -0700 (Mon, 28 May 2012)
Log Message
DFG should not generate code for code that the CFA proves to be unreachable
https://bugs.webkit.org/show_bug.cgi?id=87682
Reviewed by Sam Weinig.
This also fixes a small performance bug where CFA was not marking blocks
as having constants (and hence not triggering constant folding) if the only
constants were on GetLocals.
And fixing that bug revealed another bug: constant folding was assuming that
a GetLocal must be the first access to a local in a basic block. This isn't
true. The first access may be a Flush. This patch fixes that issue using the
safest approach possible, since we don't need to be clever for something that
only happens in one of our benchmarks.
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::execute):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::run):
* dfg/DFGJITCompiler.h:
(JSC::DFG::JITCompiler::noticeOSREntry):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compile):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (118714 => 118715)
--- trunk/Source/_javascript_Core/ChangeLog 2012-05-29 01:50:07 UTC (rev 118714)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-05-29 02:43:30 UTC (rev 118715)
@@ -1,3 +1,29 @@
+2012-05-28 Filip Pizlo <fpi...@apple.com>
+
+ DFG should not generate code for code that the CFA proves to be unreachable
+ https://bugs.webkit.org/show_bug.cgi?id=87682
+
+ Reviewed by Sam Weinig.
+
+ This also fixes a small performance bug where CFA was not marking blocks
+ as having constants (and hence not triggering constant folding) if the only
+ constants were on GetLocals.
+
+ And fixing that bug revealed another bug: constant folding was assuming that
+ a GetLocal must be the first access to a local in a basic block. This isn't
+ true. The first access may be a Flush. This patch fixes that issue using the
+ safest approach possible, since we don't need to be clever for something that
+ only happens in one of our benchmarks.
+
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::execute):
+ * dfg/DFGConstantFoldingPhase.cpp:
+ (JSC::DFG::ConstantFoldingPhase::run):
+ * dfg/DFGJITCompiler.h:
+ (JSC::DFG::JITCompiler::noticeOSREntry):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+
2012-05-28 Carlos Garcia Campos <cgar...@igalia.com>
Unreviewed. Fix make distcheck.
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (118714 => 118715)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2012-05-29 01:50:07 UTC (rev 118714)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2012-05-29 02:43:30 UTC (rev 118715)
@@ -279,6 +279,8 @@
AbstractValue value = m_variables.operand(variableAccessData->local());
if (value.isClear())
canExit |= true;
+ if (value.value())
+ m_foundConstants = true;
forNode(nodeIndex) = value;
}
node.setCanExit(canExit);
Modified: trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (118714 => 118715)
--- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2012-05-29 01:50:07 UTC (rev 118714)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2012-05-29 02:43:30 UTC (rev 118715)
@@ -77,13 +77,42 @@
ASSERT(m_graph[node.child1()].op() == Phi);
ASSERT(!m_graph[node.child1()].hasResult());
- ASSERT(block->variablesAtHead.operand(node.local()) == nodeIndex);
- ASSERT(block->isInPhis(node.child1().index()));
- block->variablesAtHead.operand(node.local()) = node.child1().index();
+ NodeIndex previousLocalAccess = NoNode;
+ if (block->variablesAtHead.operand(node.local()) == nodeIndex) {
+ // We expect this to be the common case.
+ ASSERT(block->isInPhis(node.child1().index()));
+ previousLocalAccess = node.child1().index();
+ block->variablesAtHead.operand(node.local()) = previousLocalAccess;
+ } else {
+ ASSERT(indexInBlock > 0);
+ // Must search for the previous access to this local.
+ for (BlockIndex subIndexInBlock = indexInBlock - 1; subIndexInBlock--;) {
+ NodeIndex subNodeIndex = block->at(subIndexInBlock);
+ Node& subNode = m_graph[subNodeIndex];
+ if (!subNode.shouldGenerate())
+ continue;
+ if (!subNode.hasVariableAccessData())
+ continue;
+ if (subNode.local() != node.local())
+ continue;
+ // The two must have been unified.
+ ASSERT(subNode.variableAccessData() == node.variableAccessData());
+ // Currently, the previous node must be a flush.
+ // NOTE: This assertion should be removed if we ever do
+ // constant folding on captured variables. In particular,
+ // this code does not require the previous node to be a flush,
+ // but we are asserting this anyway because it is a constraint
+ // of the IR and this is as good a place as any to assert it.
+ ASSERT(subNode.op() == Flush);
+ previousLocalAccess = subNodeIndex;
+ break;
+ }
+ ASSERT(previousLocalAccess != NoNode);
+ }
NodeIndex tailNodeIndex = block->variablesAtTail.operand(node.local());
if (tailNodeIndex == nodeIndex)
- block->variablesAtTail.operand(node.local()) = node.child1().index();
+ block->variablesAtTail.operand(node.local()) = previousLocalAccess;
else {
ASSERT(m_graph[tailNodeIndex].op() == Flush
|| m_graph[tailNodeIndex].op() == SetLocal);
Modified: trunk/Source/_javascript_Core/dfg/DFGJITCompiler.h (118714 => 118715)
--- trunk/Source/_javascript_Core/dfg/DFGJITCompiler.h 2012-05-29 01:50:07 UTC (rev 118714)
+++ trunk/Source/_javascript_Core/dfg/DFGJITCompiler.h 2012-05-29 02:43:30 UTC (rev 118715)
@@ -304,6 +304,10 @@
void noticeOSREntry(BasicBlock& basicBlock, JITCompiler::Label blockHead, LinkBuffer& linkBuffer)
{
#if DFG_ENABLE(OSR_ENTRY)
+ // OSR entry is not allowed into blocks deemed unreachable by control flow analysis.
+ if (!basicBlock.cfaHasVisited)
+ return;
+
OSREntryData* entry = codeBlock()->appendDFGOSREntryData(basicBlock.bytecodeBegin, linkBuffer.offsetOf(blockHead));
entry->m_expectedValues = basicBlock.valuesAtHead;
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (118714 => 118715)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-05-29 01:50:07 UTC (rev 118714)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-05-29 02:43:30 UTC (rev 118715)
@@ -967,6 +967,16 @@
if (!block.isReachable)
return;
+
+ if (!block.cfaHasVisited) {
+ // Don't generate code for basic blocks that are unreachable according to CFA.
+ // But to be sure that nobody has generated a jump to this block, drop in a
+ // breakpoint here.
+#if !ASSERT_DISABLED
+ m_jit.breakpoint();
+#endif
+ return;
+ }
m_blockHeads[m_block] = m_jit.label();
#if DFG_ENABLE(JIT_BREAK_ON_EVERY_BLOCK)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
