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
diff --git a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
index b860a73..3bdacef 100644
--- a/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
+++ b/Source/JavaScriptCore/dfg/DFGAbstractState.cpp
@@ -147,7 +147,13 @@
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 @@
!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 @@
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 @@
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:
