REGRESSION(r184260): arguments elimination has stopped working because of Check(Untyp...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 17:39:02 +0000 (17:39 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 17:39:02 +0000 (17:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144951

Reviewed by Michael Saboff.

There were two issues here:

- In r184260 we expected a small number of possible use kinds in Check nodes, and
  UntypedUse was not one of them. That seemed like a sensible assumption because we don't
  create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a
  Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to
  follow the same idiom as everyone else and not create tautological checks.

- It's clearly not very robust to assume that Checks will not be used tautologically. So,
  this changes how we validate Checks in the escape analyses. We now use willHaveCheck,
  which catches cases that AI would have already marked as unnecessary. It then also uses
  a new helper called alreadyChecked(), which allows us to just ask if the check is
  unnecessary for objects. That's a good fall-back in case AI hadn't run yet.

* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGMayExit.cpp:
* dfg/DFGObjectAllocationSinkingPhase.cpp:
(JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
* dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run):
* dfg/DFGUseKind.h:
(JSC::DFG::alreadyChecked):
* dfg/DFGVarargsForwardingPhase.cpp:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Source/JavaScriptCore/dfg/DFGMayExit.cpp
Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
Source/JavaScriptCore/dfg/DFGUseKind.h
Source/JavaScriptCore/dfg/DFGVarargsForwardingPhase.cpp

index c167090..e59a9a0 100644 (file)
@@ -1,3 +1,35 @@
+2015-05-13  Filip Pizlo  <fpizlo@apple.com>
+
+        REGRESSION(r184260): arguments elimination has stopped working because of Check(UntypedUse:) from SSAConversionPhase
+        https://bugs.webkit.org/show_bug.cgi?id=144951
+
+        Reviewed by Michael Saboff.
+        
+        There were two issues here:
+        
+        - In r184260 we expected a small number of possible use kinds in Check nodes, and
+          UntypedUse was not one of them. That seemed like a sensible assumption because we don't
+          create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a
+          Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to
+          follow the same idiom as everyone else and not create tautological checks.
+        
+        - It's clearly not very robust to assume that Checks will not be used tautologically. So,
+          this changes how we validate Checks in the escape analyses. We now use willHaveCheck,
+          which catches cases that AI would have already marked as unnecessary. It then also uses
+          a new helper called alreadyChecked(), which allows us to just ask if the check is
+          unnecessary for objects. That's a good fall-back in case AI hadn't run yet.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+        * dfg/DFGMayExit.cpp:
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+        (JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
+        * dfg/DFGSSAConversionPhase.cpp:
+        (JSC::DFG::SSAConversionPhase::run):
+        * dfg/DFGUseKind.h:
+        (JSC::DFG::alreadyChecked):
+        * dfg/DFGVarargsForwardingPhase.cpp:
+
+k
 2015-05-13  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [ES6] Implement String.raw
index e146c46..9400143 100644 (file)
@@ -63,6 +63,11 @@ public:
         // version over LoadStore.
         DFG_ASSERT(m_graph, nullptr, m_graph.m_form == SSA);
         
+        if (verbose) {
+            dataLog("Graph before arguments elimination:\n");
+            m_graph.dump();
+        }
+        
         identifyCandidates();
         if (m_candidates.isEmpty())
             return false;
@@ -169,15 +174,13 @@ private:
                     m_graph.doToChildren(
                         node,
                         [&] (Edge edge) {
-                            switch (edge.useKind()) {
-                            case CellUse:
-                            case ObjectUse:
-                                break;
-                                
-                            default:
-                                escape(edge);
-                                break;
-                            }
+                            if (edge.willNotHaveCheck())
+                                return;
+                            
+                            if (alreadyChecked(edge.useKind(), SpecObject))
+                                return;
+                            
+                            escape(edge);
                         });
                     break;
                     
index 22ff607..4631ecd 100644 (file)
@@ -51,12 +51,21 @@ public:
         }
 
         switch (edge.useKind()) {
+        // These are shady because nodes that have these use kinds will typically exit for
+        // unrelated reasons. For example CompareEq doesn't usually exit, but if it uses ObjectUse
+        // then it will.
         case ObjectUse:
         case ObjectOrOtherUse:
+            m_result = true;
+            break;
+            
+        // These are shady because they check the structure even if the type of the child node
+        // passes the StringObject type filter.
         case StringObjectUse:
         case StringOrStringObjectUse:
             m_result = true;
             break;
+            
         default:
             break;
         }
index 45c96dc..f7d2383 100644 (file)
@@ -841,28 +841,13 @@ private:
             m_graph.doToChildren(
                 node,
                 [&] (Edge edge) {
-                    bool ok = true;
-
-                    switch (edge.useKind()) {
-                    case KnownCellUse:
-                    case CellUse:
-                    case ObjectUse:
-                        // All of our allocations will pass this.
-                        break;
-                        
-                    case FunctionUse:
-                        // Function allocations will pass this.
-                        if (edge->op() != NewFunction)
-                            ok = false;
-                        break;
-                        
-                    default:
-                        ok = false;
-                        break;
-                    }
+                    if (edge.willNotHaveCheck())
+                        return;
                     
-                    if (!ok)
-                        escape(edge.node());
+                    if (alreadyChecked(edge.useKind(), SpecObject))
+                        return;
+                    
+                    escape(edge.node());
                 });
             break;
 
index 0745d14..6993bfc 100644 (file)
@@ -276,17 +276,18 @@ public:
                     
                 case SetLocal: {
                     VariableAccessData* variable = node->variableAccessData();
+                    Node* child = node->child1().node();
                     
                     if (!!(node->flags() & NodeIsFlushed)) {
                         node->convertToPutStack(
                             m_graph.m_stackAccessData.add(
                                 variable->local(), variable->flushFormat()));
                     } else
-                        node->setOpAndDefaultFlags(Check);
+                        node->remove();
                     
                     if (verbose)
-                        dataLog("Mapping: ", variable->local(), " -> ", node->child1().node(), "\n");
-                    valueForOperand.operand(variable->local()) = node->child1().node();
+                        dataLog("Mapping: ", variable->local(), " -> ", child, "\n");
+                    valueForOperand.operand(variable->local()) = child;
                     break;
                 }
                     
index 75227a6..25e4c54 100644 (file)
@@ -213,6 +213,17 @@ inline bool usesStructure(UseKind kind)
     }
 }
 
+// Returns true if we've already guaranteed the type 
+inline bool alreadyChecked(UseKind kind, SpeculatedType type)
+{
+    // If the check involves the structure then we need to know more than just the type to be sure
+    // that the check is done.
+    if (usesStructure(kind))
+        return false;
+    
+    return !(type & ~typeFilterFor(kind));
+}
+
 inline UseKind useKindForResult(NodeFlags result)
 {
     ASSERT(!(result & ~NodeResultMask));
index 21050a8..9371de5 100644 (file)
@@ -109,16 +109,16 @@ private:
                 m_graph.doToChildren(
                     node,
                     [&] (Edge edge) {
-                        switch (edge.useKind()) {
-                        case CellUse:
-                        case ObjectUse:
-                            if (edge == candidate)
-                                lastUserIndex = nodeIndex;
-                            break;
-                        default:
-                            sawEscape = true;
-                            break;
-                        }  
+                        if (edge == candidate)
+                            lastUserIndex = nodeIndex;
+                        
+                        if (edge.willNotHaveCheck())
+                            return;
+                        
+                        if (alreadyChecked(edge.useKind(), SpecObject))
+                            return;
+                        
+                        sawEscape = true;
                     });
                 if (sawEscape) {
                     if (verbose)