Commit 6e0a9edd authored by fpizlo@apple.com's avatar fpizlo@apple.com

Structure check hoisting fails to consider the possibility of conflicting...

Structure check hoisting fails to consider the possibility of conflicting checks on the source of the first assignment to the hoisted variable
https://bugs.webkit.org/show_bug.cgi?id=96872

Reviewed by Oliver Hunt.

This does a few related things:
        
- It turns off the use of ForceOSRExit for sure-to-fail CheckStructures, because
  I noticed that this would sometimes happen for a ForwardCheckStructure. The
  problem is that ForceOSRExit exits backwards, not forwards. Since the code that
  led to those ForceOSRExit's being inserted was written out of paranoia rather
  than need, I removed it. Specifically, I removed the m_isValid = false code
  for CheckStructure/StructureTransitionWatchpoint in AbstractState.
        
- If a structure check causes a structure set to go empty, we don't want a
  PutStructure to revive the set. It should instead be smart enough to realize 
  that an empty set implies that the code can't execute. This was the only "bug"
  that the use of m_isValid = false was preventing.
        
- Finally, the main change: structure check hoisting looks at the source of the
  SetLocals on structure-check-hoistable variables and ensures that the source
  is not checked with a conflicting structure. This is O(n^2) but it does not
  show up at all in performance tests.
        
The first two parts of this change were auxiliary bugs that were revealed by
the structure check hoister doing bad things.

* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::initialize):
(JSC::DFG::AbstractState::execute):
* dfg/DFGStructureCheckHoistingPhase.cpp:
(JSC::DFG::StructureCheckHoistingPhase::run):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@128699 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 13488d85
2012-09-15 Filip Pizlo <fpizlo@apple.com>
Structure check hoisting fails to consider the possibility of conflicting checks on the source of the first assignment to the hoisted variable
https://bugs.webkit.org/show_bug.cgi?id=96872
Reviewed by Oliver Hunt.
This does a few related things:
- It turns off the use of ForceOSRExit for sure-to-fail CheckStructures, because
I noticed that this would sometimes happen for a ForwardCheckStructure. The
problem is that ForceOSRExit exits backwards, not forwards. Since the code that
led to those ForceOSRExit's being inserted was written out of paranoia rather
than need, I removed it. Specifically, I removed the m_isValid = false code
for CheckStructure/StructureTransitionWatchpoint in AbstractState.
- If a structure check causes a structure set to go empty, we don't want a
PutStructure to revive the set. It should instead be smart enough to realize
that an empty set implies that the code can't execute. This was the only "bug"
that the use of m_isValid = false was preventing.
- Finally, the main change: structure check hoisting looks at the source of the
SetLocals on structure-check-hoistable variables and ensures that the source
is not checked with a conflicting structure. This is O(n^2) but it does not
show up at all in performance tests.
The first two parts of this change were auxiliary bugs that were revealed by
the structure check hoister doing bad things.
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::initialize):
(JSC::DFG::AbstractState::execute):
* dfg/DFGStructureCheckHoistingPhase.cpp:
(JSC::DFG::StructureCheckHoistingPhase::run):
2012-09-14 Filip Pizlo <fpizlo@apple.com>
All of the things in SparseArrayValueMap should be out-of-line
......
......@@ -147,7 +147,13 @@ void AbstractState::initialize(Graph& graph)
for (size_t i = 0; i < graph.m_mustHandleValues.size(); ++i) {
AbstractValue value;
value.setMostSpecific(graph.m_mustHandleValues[i]);
block->valuesAtHead.operand(graph.m_mustHandleValues.operandForIndex(i)).merge(value);
int operand = graph.m_mustHandleValues.operandForIndex(i);
block->valuesAtHead.operand(operand).merge(value);
#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
dataLog(" Initializing Block #%u, operand r%d, to ", blockIndex, operand);
block->valuesAtHead.operand(operand).dump(WTF::dataFile());
dataLog("\n");
#endif
}
block->cfaShouldRevisit = true;
}
......@@ -1293,20 +1299,6 @@ bool AbstractState::execute(unsigned indexInBlock)
!value.m_currentKnownStructure.isSubsetOf(set)
|| !isCellSpeculation(value.m_type));
value.filter(set);
// This is likely to be unnecessary, but it's conservative, and that's a good thing.
// This is trying to avoid situations where the CFA proves that this structure check
// must fail due to a future structure proof. We have two options at that point. We
// can either compile all subsequent code as we would otherwise, or we can ensure
// that the subsequent code is never reachable. The former is correct because the
// Proof Is Infallible (TM) -- hence even if we don't force the subsequent code to
// be unreachable, it must be unreachable nonetheless. But imagine what would happen
// if the proof was borked. In the former case, we'd get really bizarre bugs where
// we assumed that the structure of this object was known even though it wasn't. In
// the latter case, we'd have a slight performance pathology because this would be
// turned into an OSR exit unnecessarily. Which would you rather have?
if (value.m_currentKnownStructure.isClear()
|| value.m_futurePossibleStructure.isClear())
m_isValid = false;
m_haveStructures = true;
break;
}
......@@ -1325,10 +1317,6 @@ bool AbstractState::execute(unsigned indexInBlock)
ASSERT(value.isClear() || isCellSpeculation(value.m_type)); // Value could be clear if we've proven must-exit due to a speculation statically known to be bad.
value.filter(node.structure());
// See comment in CheckStructure for why this is here.
if (value.m_currentKnownStructure.isClear()
|| value.m_futurePossibleStructure.isClear())
m_isValid = false;
m_haveStructures = true;
node.setCanExit(true);
break;
......@@ -1337,9 +1325,11 @@ bool AbstractState::execute(unsigned indexInBlock)
case PutStructure:
case PhantomPutStructure:
node.setCanExit(false);
clobberStructures(indexInBlock);
forNode(node.child1()).set(node.structureTransitionData().newStructure);
m_haveStructures = true;
if (!forNode(node.child1()).m_currentKnownStructure.isClear()) {
clobberStructures(indexInBlock);
forNode(node.child1()).set(node.structureTransitionData().newStructure);
m_haveStructures = true;
}
break;
case GetButterfly:
case AllocatePropertyStorage:
......
......@@ -67,7 +67,8 @@ public:
if (!node.shouldGenerate())
continue;
switch (node.op()) {
case CheckStructure: {
case CheckStructure:
case StructureTransitionWatchpoint: {
Node& child = m_graph[node.child1()];
if (child.op() != GetLocal)
break;
......@@ -91,7 +92,6 @@ public:
case GetByOffset:
case PutByOffset:
case PutStructure:
case StructureTransitionWatchpoint:
case AllocatePropertyStorage:
case ReallocatePropertyStorage:
case GetButterfly:
......@@ -105,6 +105,40 @@ public:
// Don't count these uses.
break;
case SetLocal: {
// Find all uses of the source of the SetLocal. If any of them are a
// kind of CheckStructure, then we should notice them to ensure that
// we're not hoisting a check that would contravene checks that are
// already being performed.
VariableAccessData* variable = node.variableAccessData();
if (variable->isCaptured() || variable->structureCheckHoistingFailed())
break;
if (!isCellSpeculation(variable->prediction()))
break;
NodeIndex source = node.child1().index();
for (unsigned subIndexInBlock = 0; subIndexInBlock < block->size(); ++subIndexInBlock) {
NodeIndex subNodeIndex = block->at(subIndexInBlock);
Node& subNode = m_graph[subNodeIndex];
if (!subNode.shouldGenerate())
continue;
switch (subNode.op()) {
case CheckStructure:
case StructureTransitionWatchpoint: {
if (subNode.child1().index() != source)
break;
noticeStructureCheck(variable, subNode.structureSet());
break;
}
default:
break;
}
}
m_graph.vote(node, VoteOther);
break;
}
default:
m_graph.vote(node, VoteOther);
break;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment