VarargsForwardingPhase should use bytecode liveness in addition to other uses to...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Apr 2015 18:49:15 +0000 (18:49 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Apr 2015 18:49:15 +0000 (18:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143843

Reviewed by Geoffrey Garen.

It will soon come to pass that Phantom isn't available at the time that
VarargsForwardingPhase runs. So, it needs to use some other mechanism for discovering when
a value dies for OSR.

This is simplified by two things:

1) The bytecode kill analysis is now reusable. This patch makes it even more reusable than
   before by polishing the API.

2) This phase already operates on one node at a time and allows itself to do a full search
   of the enclosing basic block for that node. This is fine because CreateDirectArguments
   and friends is a rarely occurring node. The fact that it operates on one node at a time
   makes it even easier to reason about OSR liveness - we just track the list of locals in
   which it is live.

This change has no effect right now but it is a necessary prerequisite to implementing
https://bugs.webkit.org/show_bug.cgi?id=143736.

* dfg/DFGBasicBlock.h:
(JSC::DFG::BasicBlock::tryAt):
* dfg/DFGForAllKills.h:
(JSC::DFG::forAllKilledOperands):
* dfg/DFGPhantomInsertionPhase.cpp:
* dfg/DFGVarargsForwardingPhase.cpp:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGBasicBlock.h
Source/JavaScriptCore/dfg/DFGForAllKills.h
Source/JavaScriptCore/dfg/DFGPhantomInsertionPhase.cpp
Source/JavaScriptCore/dfg/DFGVarargsForwardingPhase.cpp

index 9613758..f984c45 100644 (file)
@@ -1,3 +1,35 @@
+2015-04-25  Filip Pizlo  <fpizlo@apple.com>
+
+        VarargsForwardingPhase should use bytecode liveness in addition to other uses to determine the last point that a candidate is used
+        https://bugs.webkit.org/show_bug.cgi?id=143843
+
+        Reviewed by Geoffrey Garen.
+        
+        It will soon come to pass that Phantom isn't available at the time that
+        VarargsForwardingPhase runs. So, it needs to use some other mechanism for discovering when
+        a value dies for OSR.
+        
+        This is simplified by two things:
+        
+        1) The bytecode kill analysis is now reusable. This patch makes it even more reusable than
+           before by polishing the API.
+        
+        2) This phase already operates on one node at a time and allows itself to do a full search
+           of the enclosing basic block for that node. This is fine because CreateDirectArguments
+           and friends is a rarely occurring node. The fact that it operates on one node at a time
+           makes it even easier to reason about OSR liveness - we just track the list of locals in
+           which it is live.
+        
+        This change has no effect right now but it is a necessary prerequisite to implementing
+        https://bugs.webkit.org/show_bug.cgi?id=143736.
+
+        * dfg/DFGBasicBlock.h:
+        (JSC::DFG::BasicBlock::tryAt):
+        * dfg/DFGForAllKills.h:
+        (JSC::DFG::forAllKilledOperands):
+        * dfg/DFGPhantomInsertionPhase.cpp:
+        * dfg/DFGVarargsForwardingPhase.cpp:
+
 2015-04-27  Jordan Harband  <ljharb@gmail.com>
 
         Map#entries and Map#keys error for non-Maps is swapped
index 8fc33c6..f0d21e6 100644 (file)
@@ -61,6 +61,12 @@ struct BasicBlock : RefCounted<BasicBlock> {
     bool isEmpty() const { return !size(); }
     Node*& at(size_t i) { return m_nodes[i]; }
     Node* at(size_t i) const { return m_nodes[i]; }
+    Node* tryAt(size_t i) const
+    {
+        if (i >= size())
+            return nullptr;
+        return at(i);
+    }
     Node*& operator[](size_t i) { return at(i); }
     Node* operator[](size_t i) const { return at(i); }
     
index 33dafb8..55d6611 100644 (file)
@@ -75,6 +75,12 @@ template<typename Functor>
 void forAllKilledOperands(Graph& graph, Node* nodeBefore, Node* nodeAfter, const Functor& functor)
 {
     CodeOrigin before = nodeBefore->origin.forExit;
+
+    if (!nodeAfter) {
+        graph.forAllLiveInBytecode(before, functor);
+        return;
+    }
+    
     CodeOrigin after = nodeAfter->origin.forExit;
     
     VirtualRegister alreadyNoted;
index 7171698..cff1a40 100644 (file)
@@ -122,42 +122,35 @@ private:
             
             node->setEpoch(currentEpoch);
 
-            auto killAction = [&] (VirtualRegister reg) {
-                if (verbose)
-                    dataLog("    Killed operand: ", reg, "\n");
-                        
-                Node* killedNode = m_values.operand(reg);
-                if (!killedNode)
-                    return;
-                
-                // We only need to insert a Phantom if the node hasn't been used since the last
-                // exit, and was born before the last exit.
-                if (killedNode->epoch() == currentEpoch)
-                    return;
-                
-                if (verbose) {
-                    dataLog(
-                        "    Inserting Phantom on ", killedNode, " after ",
-                        block->at(lastExitingIndex), "\n");
-                }
-                
-                // We have exact ref counts, so creating a new use means that we have to increment
-                // the ref count.
-                killedNode->postfixRef();
-                
-                m_insertionSet.insertNode(
-                    lastExitingIndex + 1, SpecNone, Phantom, block->at(lastExitingIndex)->origin,
-                    killedNode->defaultEdge());
-            };
-            
-            if (nodeIndex + 1 == block->size()) {
-                // Should a MovHinted value be kept alive? If the value has been SetLocal'd then
-                // the answer is no. But we may have a value that is live here and dead in
-                // successors because we had jettisoned those successors that would have used the
-                // value. Hence, anything live here should be kept alive.
-                m_graph.forAllLiveInBytecode(node->origin.forExit, killAction);
-            } else
-                forAllKilledOperands(m_graph, node, block->at(nodeIndex + 1), killAction);
+            forAllKilledOperands(
+                m_graph, node, block->tryAt(nodeIndex + 1),
+                [&] (VirtualRegister reg) {
+                    if (verbose)
+                        dataLog("    Killed operand: ", reg, "\n");
+                    
+                    Node* killedNode = m_values.operand(reg);
+                    if (!killedNode)
+                        return;
+                    
+                    // We only need to insert a Phantom if the node hasn't been used since the last
+                    // exit, and was born before the last exit.
+                    if (killedNode->epoch() == currentEpoch)
+                        return;
+                    
+                    if (verbose) {
+                        dataLog(
+                            "    Inserting Phantom on ", killedNode, " after ",
+                            block->at(lastExitingIndex), "\n");
+                    }
+                    
+                    // We have exact ref counts, so creating a new use means that we have to
+                    // increment the ref count.
+                    killedNode->postfixRef();
+                    
+                    m_insertionSet.insertNode(
+                        lastExitingIndex + 1, SpecNone, Phantom,
+                        block->at(lastExitingIndex)->origin, killedNode->defaultEdge());
+            });
         }
         
         m_insertionSet.execute(block);
index c9d90d4..c73b1b2 100644 (file)
@@ -30,6 +30,7 @@
 
 #include "DFGArgumentsUtilities.h"
 #include "DFGClobberize.h"
+#include "DFGForAllKills.h"
 #include "DFGGraph.h"
 #include "DFGPhase.h"
 #include "JSCInlines.h"
@@ -49,6 +50,8 @@ public:
     
     bool run()
     {
+        DFG_ASSERT(m_graph, nullptr, m_graph.m_form != SSA);
+        
         if (verbose) {
             dataLog("Graph before varargs forwarding:\n");
             m_graph.dump();
@@ -87,13 +90,19 @@ private:
         // Find the index of the last node in this block to use the candidate, and look for escaping
         // sites.
         unsigned lastUserIndex = candidateNodeIndex;
+        Vector<VirtualRegister, 2> relevantLocals; // This is a set. We expect it to be a small set.
         for (unsigned nodeIndex = candidateNodeIndex + 1; nodeIndex < block->size(); ++nodeIndex) {
             Node* node = block->at(nodeIndex);
+            
             switch (node->op()) {
+            case MovHint:
+                lastUserIndex = nodeIndex;
+                if (!relevantLocals.contains(node->unlinkedLocal()))
+                    relevantLocals.append(node->unlinkedLocal());
+                break;
+                
             case Phantom:
             case Check:
-            case MovHint:
-            case PutHint:
             case LoadVarargs:
                 if (m_graph.uses(node, candidate))
                     lastUserIndex = nodeIndex;
@@ -125,6 +134,18 @@ private:
                     return;
                 }
             }
+            
+            forAllKilledOperands(
+                m_graph, node, block->tryAt(nodeIndex + 1),
+                [&] (VirtualRegister reg) {
+                    for (unsigned i = 0; i < relevantLocals.size(); ++i) {
+                        if (relevantLocals[i] == reg) {
+                            relevantLocals[i--] = relevantLocals.last();
+                            relevantLocals.removeLast();
+                            lastUserIndex = nodeIndex;
+                        }
+                    }
+                });
         }
         if (verbose)
             dataLog("Selected lastUserIndex = ", lastUserIndex, ", ", block->at(lastUserIndex), "\n");