Changeset 128699 in webkit for trunk/Source/JavaScriptCore/dfg/DFGAbstractState.cpp

Timestamp:

Sep 15, 2012, 7:36:22 PM (13 years ago)

Author:

[email protected]

Message:

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):

File:

• trunk/Source/JavaScriptCore/dfg/DFGAbstractState.cpp (modified) (4 diffs)

trunk/Source/JavaScriptCore/dfg/DFGAbstractState.cpp

@@ -148 +148 @@
             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;
@@ -1294 +1300 @@
             || !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;
@@ -1326 +1318 @@
         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);
@@ -1338 +1326 @@
     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: