Changeset 171190 in webkit for trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

Timestamp:

Jul 17, 2014, 11:17:43 AM (11 years ago)

Author:

[email protected]

Message:

DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
​https://bugs.webkit.org/show_bug.cgi?id=134988
<rdar://problem/17706349>

Reviewed by Oliver Hunt.

Luckily, we also don't need this optimization to be super powerful: the only place
where it really matters is for getting rid of the redundancy between op_enter and
op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
two things. This change updates the store eliminator to know about only that small,
obviously safe, set of nodes over which we can store-eliminate.

This shouldn't have any performance impact in the DFG because this optimization kicks
in relatively rarely already. And once we tier up into the FTL, we get a much better
store elimination over LLVM IR, so this really shouldn't matter at all.

The tricky part of this patch is that there is a close relative of this optimization,
for uncaptured variables that got flushed. This happens for arguments to inlined calls.
I make this work by splitting it into two different store eliminators.

Note that in the process of crafting the tests, I realized that we were incorrectly
DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
to a check node followed by the actual allocation.

• dfg/DFGCSEPhase.cpp:

(JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::setLocalStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE):
(JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.

• dfg/DFGNodeType.h:
• tests/stress/capture-escape-and-throw.js: Added.

(foo.f):
(foo):

• tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.

(foo):
(bar):

File:

• trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp (modified) (9 diffs)

trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

@@ -929 +929 @@
     }
     
-    struct SetLocalStoreEliminationResult {
-        SetLocalStoreEliminationResult()
-            : mayBeAccessed(false)
-            , mayExit(false)
-            , mayClobberWorld(false)
-        {
-        }
-        
-        bool mayBeAccessed;
-        bool mayExit;
-        bool mayClobberWorld;
-    };
-    SetLocalStoreEliminationResult setLocalStoreElimination(
-        VirtualRegister local, Node* expectedNode)
-    {
-        SetLocalStoreEliminationResult result;
+    bool uncapturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
+    {
         for (unsigned i = m_indexInBlock; i--;) {
             Node* node = m_currentBlock->at(i);
@@ -951 +937 @@
             case Flush:
                 if (node->local() == local)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
             case GetLocalUnlinked:
                 if (node->unlinkedLocal() == local)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
@@ -963 +949 @@
                     break;
                 if (node != expectedNode)
-                    result.mayBeAccessed = true;
-                return result;
+                    return false;
+                return true;
             }
                 
@@ -970 +956 @@
             case PutClosureVar:
                 if (static_cast<VirtualRegister>(node->varNumber()) == local)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
@@ -978 +964 @@
                     break;
                 if (m_graph.uncheckedActivationRegister() == local)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
@@ -985 +971 @@
             case GetMyArgumentsLengthSafe:
                 if (m_graph.uncheckedArgumentsRegisterFor(node->origin.semantic) == local)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
             case GetMyArgumentByVal:
             case GetMyArgumentByValSafe:
-                result.mayBeAccessed = true;
-                break;
+                return false;
                 
             case GetByVal:
                 // If this is accessing arguments then it's potentially accessing locals.
                 if (node->arrayMode().type() == Array::Arguments)
-                    result.mayBeAccessed = true;
+                    return false;
                 break;
                 
@@ -1006 +991 @@
                 // argument register. But that seems like it would buy us very little since
                 // any kind of tear offs are rare to begin with.
-                result.mayBeAccessed = true;
-                break;
-                
-            default:
-                break;
-            }
-            result.mayExit |= node->canExit();
-            result.mayClobberWorld |= m_graph.clobbersWorld(node);
+                return false;
+                
+            default:
+                break;
+            }
+            if (m_graph.clobbersWorld(node))
+                return false;
         }
         RELEASE_ASSERT_NOT_REACHED();
-        // Be safe in release mode.
-        result.mayBeAccessed = true;
-        return result;
+        return false;
+    }
+
+    bool capturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
+    {
+        for (unsigned i = m_indexInBlock; i--;) {
+            Node* node = m_currentBlock->at(i);
+            switch (node->op()) {
+            case GetLocal:
+            case Flush:
+                if (node->local() == local)
+                    return false;
+                break;
+                
+            case GetLocalUnlinked:
+                if (node->unlinkedLocal() == local)
+                    return false;
+                break;
+                
+            case SetLocal: {
+                if (node->local() != local)
+                    break;
+                if (node != expectedNode)
+                    return false;
+                return true;
+            }
+                
+            case Phantom:
+            case Check:
+            case HardPhantom:
+            case MovHint:
+            case JSConstant:
+            case DoubleConstant:
+            case Int52Constant:
+                break;
+                
+            default:
+                return false;
+            }
+        }
+        RELEASE_ASSERT_NOT_REACHED();
+        return false;
+    }
+    
+    bool setLocalStoreElimination(VariableAccessData* variableAccessData, Node* expectedNode)
+    {
+        if (variableAccessData->isCaptured())
+            return capturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
+        return uncapturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
     }
     
@@ -1216 +1246 @@
             ASSERT(m_graph.m_form != SSA);
             VariableAccessData* variableAccessData = node->variableAccessData();
-            VirtualRegister local = variableAccessData->local();
             if (!node->child1()) {
                 // FIXME: It's silly that we punt on flush-eliminating here. We don't really
@@ -1240 +1269 @@
                     break;
             }
-            SetLocalStoreEliminationResult result =
-                setLocalStoreElimination(local, replacement);
-            if (result.mayBeAccessed || result.mayClobberWorld)
+            if (!setLocalStoreElimination(variableAccessData, replacement))
                 break;
             ASSERT(replacement->op() == SetLocal);
-            // FIXME: Investigate using mayExit as a further optimization.
             node->convertToPhantom();
             Node* dataNode = replacement->child1().node();