Changeset 128699 in webkit for trunk/Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.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/DFGStructureCheckHoistingPhase.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp

@@ -68 +68 @@
                     continue;
                 switch (node.op()) {
-                case CheckStructure: {
+                case CheckStructure:
+                case StructureTransitionWatchpoint: {
                     Node& child = m_graph[node.child1()];
                     if (child.op() != GetLocal)
@@ -92 +93 @@
                 case PutByOffset:
                 case PutStructure:
-                case StructureTransitionWatchpoint:
                 case AllocatePropertyStorage:
                 case ReallocatePropertyStorage:
@@ -105 +105 @@
                     // 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: