DFG::ByteCodeParser shouldn't call tryGetConstantProperty() with some StructureSet...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Aug 2015 21:04:00 +0000 (21:04 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Aug 2015 21:04:00 +0000 (21:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=147891
rdar://problem/22129447

Reviewed by Mark Lam.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleGetByOffset): Get rid of this.
(JSC::DFG::ByteCodeParser::load): Don't call the version of handleGetByOffset() that assumes that we had CheckStructure'd some StructureSet, since we may not have CheckStructure'd anything.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::assertIsRegistered): Make this always assert even before the StructureRegistrationPhase.
* dfg/DFGStructureRegistrationPhase.cpp:
(JSC::DFG::StructureRegistrationPhase::run): Add a FIXME that notes that we no longer believe that structures should be registered only at this phase. They should be registered before this phase and this phase should be removed.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@188292 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp

index f0216d3..a2974c0 100644 (file)
@@ -1,3 +1,19 @@
+2015-08-11  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG::ByteCodeParser shouldn't call tryGetConstantProperty() with some StructureSet if it isn't checking that the base has a structure in that StructureSet
+        https://bugs.webkit.org/show_bug.cgi?id=147891
+        rdar://problem/22129447
+
+        Reviewed by Mark Lam.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleGetByOffset): Get rid of this.
+        (JSC::DFG::ByteCodeParser::load): Don't call the version of handleGetByOffset() that assumes that we had CheckStructure'd some StructureSet, since we may not have CheckStructure'd anything.
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::assertIsRegistered): Make this always assert even before the StructureRegistrationPhase.
+        * dfg/DFGStructureRegistrationPhase.cpp:
+        (JSC::DFG::StructureRegistrationPhase::run): Add a FIXME that notes that we no longer believe that structures should be registered only at this phase. They should be registered before this phase and this phase should be removed.
+
 2015-08-11  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Switch Windows build to Visual Studio 2015
index 56422b1..d69e100 100644 (file)
@@ -204,7 +204,6 @@ private:
     bool handleConstantInternalFunction(int resultOperand, InternalFunction*, int registerOffset, int argumentCountIncludingThis, CodeSpecializationKind, const ChecksFunctor& insertChecks);
     Node* handlePutByOffset(Node* base, unsigned identifier, PropertyOffset, Node* value);
     Node* handleGetByOffset(SpeculatedType, Node* base, unsigned identifierNumber, PropertyOffset, NodeType = GetByOffset);
-    Node* handleGetByOffset(SpeculatedType, Node* base, const StructureSet&, unsigned identifierNumber, PropertyOffset, NodeType = GetByOffset);
     Node* handleGetByOffset(SpeculatedType, Node* base, UniquedStringImpl*, PropertyOffset, NodeType = GetByOffset);
 
     // Create a presence ObjectPropertyCondition based on some known offset and structure set. Does not
@@ -2286,18 +2285,6 @@ Node* ByteCodeParser::handleGetByOffset(SpeculatedType prediction, Node* base, u
     return getByOffset;
 }
 
-Node* ByteCodeParser::handleGetByOffset(SpeculatedType prediction, Node* base, const StructureSet& structureSet, unsigned identifierNumber, PropertyOffset offset, NodeType op)
-{
-    if (base->hasConstant()) {
-        if (JSValue constant = m_graph.tryGetConstantProperty(base->asJSValue(), structureSet, offset)) {
-            addToGraph(Phantom, base);
-            return weakJSConstant(constant);
-        }
-    }
-    
-    return handleGetByOffset(prediction, base, identifierNumber, offset, op);
-}
-
 Node* ByteCodeParser::handlePutByOffset(Node* base, unsigned identifier, PropertyOffset offset, Node* value)
 {
     Node* propertyStorage;
@@ -2593,8 +2580,7 @@ Node* ByteCodeParser::load(
         loadedValue = load(loadPrediction, variant.conditionSet(), loadOp);
     else {
         loadedValue = handleGetByOffset(
-            loadPrediction, base, variant.structureSet(), identifierNumber, variant.offset(),
-            loadOp);
+            loadPrediction, base, identifierNumber, variant.offset(), loadOp);
     }
 
     return loadedValue;
index 0ee73f4..4d013be 100644 (file)
@@ -1280,9 +1280,6 @@ void Graph::assertIsRegistered(Structure* structure)
     if (!structure)
         return;
     
-    if (m_structureRegistrationState == HaveNotStartedRegistering)
-        return;
-    
     DFG_ASSERT(*this, nullptr, m_plan.weakReferences.contains(structure));
     
     if (!structure->dfgShouldWatch())
index 3a9a488..ac9273c 100644 (file)
@@ -44,6 +44,13 @@ public:
     
     bool run()
     {
+        // FIXME: This phase shouldn't exist. We should have registered all structures by now, since
+        // we may already have done optimizations that rely on structures having been registered.
+        // Currently, we still have places where we don't register structures prior to this phase,
+        // but structures don't end up being used for optimization prior to this phase. That's a
+        // pretty fragile situation and we should fix it eventually.
+        // https://bugs.webkit.org/show_bug.cgi?id=147889
+        
         // We need to set this before this phase finishes. This phase doesn't do anything
         // conditioned on this field, except for assertIsRegistered() below. We intend for that
         // method to behave as if the phase was already finished. So, we set this up here.