It should be easy to add new nodes that do OSR forward rewiring in bo…

…th DFG and FTL

https://bugs.webkit.org/show_bug.cgi?id=121371

Reviewed by Sam Weinig.
        
Forward rewiring is a tricky part of OSR that handles the following:
        
    a: Something(...)
       SetLocal(@A, locX)
    b: Int32ToDouble(@A)
    c: SomethingThatExits(@b)
       <no further uses of @A or @b>

Note that at @c, OSR will think that locX->@A, but @A will be dead. So it must be
smart enough to find @b, which contains an equivalent value. It must do this for
any identity functions we support. Currently we support four such functions.
        
Currently the code for doing this is basically duplicated between the DFG and the
FTL. Also both versions of the code have some really weirdly written logic for
picking the "best" identity function to use.
        
We should fix this by simply having a way to ask "is this node an identity
function, and if so, then how good is it?"  Then both the DFG and FTL could use
this and have no hard-wired knowledge of those identity functions.
        
While we're at it, this also changes some terminology because I found the use of
the word "needs" confusing. Note that this retains the somewhat confusing behavior
that we don't search all possible forward/backward uses. We only search one step
in each direction. This is because we only need to handle cases that FixupPhase
and the parser insert. All other code that tries to insert intermediate conversion
nodes should ensure to Phantom the original node. For example, the following
transformation is illegal:
        
Before:
    x: SomethingThatExits(@A)
        
After:
    w: Conversion(@A)
    x: SomethingThatExits(@w)
        
The correct form of that transformation is one of these:
        
Correct #1:
        
    v: DoAllChecks(@A) // exit here
    w: Conversion(@A)
    x: Something(@w) // no exit
        
Correct #2:
        
    w: Conversion(@A)
    x: SomethingThatExits(@w)
    y: Phantom(@A)
        
Correct #3:
        
    w: Conversion(@A)
    x: SomethingThatExits(@w, @A)
        
Note that we use #3 for some heap accesses, but of course it requires that the
node you're using has an extra slot for a "dummy" use child.
        
Broadly speaking though, such transformations should be relegated to something
below DFG IR, like LLVM IR.

* dfg/DFGNodeType.h:
(JSC::DFG::forwardRewiringSelectionScore):
(JSC::DFG::needsOSRForwardRewiring):
* dfg/DFGVariableEventStream.cpp:
(JSC::DFG::VariableEventStream::reconstruct):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::addExitArgumentForNode):



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

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,78 @@
+2013-09-14  Filip Pizlo  <[email protected]>
+
+        It should be easy to add new nodes that do OSR forward rewiring in both DFG and FTL
+        https://bugs.webkit.org/show_bug.cgi?id=121371
+
+        Reviewed by Sam Weinig.
+
+        Forward rewiring is a tricky part of OSR that handles the following:
+
+            a: Something(...)
+               SetLocal(@a, locX)
+            b: Int32ToDouble(@a)
+            c: SomethingThatExits(@b)
+               <no further uses of @a or @b>
+
+        Note that at @c, OSR will think that locX->@a, but @a will be dead. So it must be
+        smart enough to find @b, which contains an equivalent value. It must do this for
+        any identity functions we support. Currently we support four such functions.
+
+        Currently the code for doing this is basically duplicated between the DFG and the
+        FTL. Also both versions of the code have some really weirdly written logic for
+        picking the "best" identity function to use.
+
+        We should fix this by simply having a way to ask "is this node an identity
+        function, and if so, then how good is it?"  Then both the DFG and FTL could use
+        this and have no hard-wired knowledge of those identity functions.
+
+        While we're at it, this also changes some terminology because I found the use of
+        the word "needs" confusing. Note that this retains the somewhat confusing behavior
+        that we don't search all possible forward/backward uses. We only search one step
+        in each direction. This is because we only need to handle cases that FixupPhase
+        and the parser insert. All other code that tries to insert intermediate conversion
+        nodes should ensure to Phantom the original node. For example, the following
+        transformation is illegal:
+
+        Before:
+            x: SomethingThatExits(@a)
+
+        After:
+            w: Conversion(@a)
+            x: SomethingThatExits(@w)
+
+        The correct form of that transformation is one of these:
+
+        Correct #1:
+
+            v: DoAllChecks(@a) // exit here
+            w: Conversion(@a)
+            x: Something(@w) // no exit
+
+        Correct #2:
+
+            w: Conversion(@a)
+            x: SomethingThatExits(@w)
+            y: Phantom(@a)
+
+        Correct #3:
+
+            w: Conversion(@a)
+            x: SomethingThatExits(@w, @a)
+
+        Note that we use #3 for some heap accesses, but of course it requires that the
+        node you're using has an extra slot for a "dummy" use child.
+
+        Broadly speaking though, such transformations should be relegated to something
+        below DFG IR, like LLVM IR.
+
+        * dfg/DFGNodeType.h:
+        (JSC::DFG::forwardRewiringSelectionScore):
+        (JSC::DFG::needsOSRForwardRewiring):
+        * dfg/DFGVariableEventStream.cpp:
+        (JSC::DFG::VariableEventStream::reconstruct):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::addExitArgumentForNode):
+
 2013-09-14  Filip Pizlo  <[email protected]>
 
         Rename IntegerBranch/IntegerCompare to Int32Branch/Int32Compare.

Source/JavaScriptCore/dfg/DFGMinifiedNode.h

@@ -50,7 +50,7 @@ inline bool belongsInMinifiedGraph(NodeType type)
     case PhantomArguments:
         return true;
     default:
-        ASSERT(!needsOSRBackwardRewiring(type) && !needsOSRForwardRewiring(type));
+        ASSERT(!permitsOSRBackwardRewiring(type) && !permitsOSRForwardRewiring(type));
         return false;
     }
 }

Source/JavaScriptCore/dfg/DFGNodeType.h

@@ -305,24 +305,67 @@ inline NodeFlags defaultFlags(NodeType op)
     }
 }
 
-inline bool needsOSRBackwardRewiring(NodeType op)
+inline bool permitsOSRBackwardRewiring(NodeType op)
 {
-    return op == UInt32ToNumber;
+    switch (op) {
+    case Identity:
+        RELEASE_ASSERT_NOT_REACHED();
+        return true;
+    case UInt32ToNumber:
+        // This is the only node where we do:
+        //
+        //     b: UInt32ToNumber(@a)
+        //     c: SetLocal(@b)
+        //
+        // and then also have some uses of @a without Phantom'ing @b.
+        return true;
+    default:
+        return false;
+    }
 }
 
-inline bool needsOSRForwardRewiring(NodeType op)
+// Returns the priority with which we should select the given node for forward
+// rewiring. Higher is better. Zero means that the node is not useful for rewiring.
+// By convention, we use 100 to mean that the node is totally equivalent to its
+// input with no information loss.
+inline unsigned forwardRewiringSelectionScore(NodeType op)
 {
     switch (op) {
+    case Identity:
+        // We shouldn't see these by the time we get to OSR even though it clearly
+        // is a perfect identity function.
+        RELEASE_ASSERT_NOT_REACHED();
+        return 100;
+
+    case DoubleAsInt32:
+        // This speculates that the incoming double is convertible to an int32. So
+        // its result is totally equivalent.
+        return 100;
+
     case Int32ToDouble:
-    case ValueToInt32:
+        // This converts an int32 to a double, but that loses a bit of information.
+        // OTOH it's still an equivalent number.
+        return 75;
+
     case UInt32ToNumber:
-    case DoubleAsInt32:
-        return true;
+        // It's completely fine to use this for OSR exit, since the uint32 isn't
+        // actually representable in bytecode.
+        return 100;
+
+    case ValueToInt32:
+        // This loses information. Only use it if there are no better alternatives.
+        return 25;
+
     default:
-        return false;
+        return 0;
     }
 }
 
+inline bool permitsOSRForwardRewiring(NodeType op)
+{
+    return forwardRewiringSelectionScore(op) > 0;
+}
+
 } } // namespace JSC::DFG
 
 #endif // ENABLE(DFG_JIT)

Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp

@@ -193,28 +193,26 @@ void VariableEventStream::reconstruct(
         MinifiedGenerationInfo info = generationInfos.get(source.id());
         if (info.format == DataFormatNone) {
             // Try to see if there is an alternate node that would contain the value we want.
-            // There are four possibilities:
             //
-            // Int32ToDouble: We can use this in place of the original node, but
-            //    we'd rather not; so we use it only if it is the only remaining
-            //    live version.
+            // Backward rewiring refers to:
             //
-            // ValueToInt32: If the only remaining live version of the value is
-            //    ValueToInt32, then we can use it.
+            //     a: Something(...)
+            //     b: Id(@a) // some identity function
+            //     c: SetLocal(@b)
             //
-            // UInt32ToNumber: If the only live version of the value is a UInt32ToNumber
-            //    then the only remaining uses are ones that want a properly formed number
-            //    rather than a UInt32 intermediate.
+            // Where we find @b being dead, but @a is still alive.
             //
-            // DoubleAsInt32: Same as UInt32ToNumber.
+            // Forward rewiring refers to:
             //
-            // The reverse of the above: This node could be a UInt32ToNumber, but its
-            //    alternative is still alive. This means that the only remaining uses of
-            //    the number would be fine with a UInt32 intermediate.
+            //     a: Something(...)
+            //     b: SetLocal(@a)
+            //     c: Id(@a) // some identity function
+            //
+            // Where we find @a being dead, but @b is still alive.
 
             bool found = false;
 
-            if (node && needsOSRBackwardRewiring(node->op())) {
+            if (node && permitsOSRBackwardRewiring(node->op())) {
                 MinifiedID id = node->child1();
                 if (tryToSetConstantRecovery(valueRecoveries[i], codeBlock, graph.at(id)))
                     continue;
@@ -224,10 +222,8 @@ void VariableEventStream::reconstruct(
             }
 
             if (!found) {
-                MinifiedID int32ToDoubleID;
-                MinifiedID valueToInt32ID;
-                MinifiedID uint32ToNumberID;
-                MinifiedID doubleAsInt32ID;
+                MinifiedID bestID;
+                unsigned bestScore = 0;
 
                 HashMap<MinifiedID, MinifiedGenerationInfo>::iterator iter = generationInfos.begin();
                 HashMap<MinifiedID, MinifiedGenerationInfo>::iterator end = generationInfos.end();
@@ -242,37 +238,15 @@ void VariableEventStream::reconstruct(
                         continue;
                     if (iter->value.format == DataFormatNone)
                         continue;
-                    switch (node->op()) {
-                    case Int32ToDouble:
-                        int32ToDoubleID = id;
-                        break;
-                    case ValueToInt32:
-                        valueToInt32ID = id;
-                        break;
-                    case UInt32ToNumber:
-                        uint32ToNumberID = id;
-                        break;
-                    case DoubleAsInt32:
-                        doubleAsInt32ID = id;
-                        break;
-                    default:
-                        ASSERT(!needsOSRForwardRewiring(node->op()));
-                        break;
-                    }
+                    unsigned myScore = forwardRewiringSelectionScore(node->op());
+                    if (myScore <= bestScore)
+                        continue;
+                    bestID = id;
+                    bestScore = myScore;
                 }
 
-                MinifiedID idToUse;
-                if (!!doubleAsInt32ID)
-                    idToUse = doubleAsInt32ID;
-                else if (!!int32ToDoubleID)
-                    idToUse = int32ToDoubleID;
-                else if (!!valueToInt32ID)
-                    idToUse = valueToInt32ID;
-                else if (!!uint32ToNumberID)
-                    idToUse = uint32ToNumberID;
-
-                if (!!idToUse) {
-                    info = generationInfos.get(idToUse);
+                if (!!bestID) {
+                    info = generationInfos.get(bestID);
                     ASSERT(info.format != DataFormatNone);
                     found = true;
                 }

Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

@@ -3087,7 +3087,7 @@ class LowerDFGToLLVM {
         if (!isLive(node)) {
             bool found = false;
 
-            if (needsOSRBackwardRewiring(node->op())) {
+            if (permitsOSRBackwardRewiring(node->op())) {
                 node = node->child1().node();
                 if (tryToSetConstantExitArgument(exit, index, node))
                     return;
@@ -3096,10 +3096,8 @@ class LowerDFGToLLVM {
             }
 
             if (!found) {
-                Node* int32ToDouble = 0;
-                Node* valueToInt32 = 0;
-                Node* uint32ToNumber = 0;
-                Node* doubleAsInt32 = 0;
+                Node* bestNode = 0;
+                unsigned bestScore = 0;
 
                 HashSet<Node*>::iterator iter = m_live.begin();
                 HashSet<Node*>::iterator end = m_live.end();
@@ -3111,36 +3109,18 @@ class LowerDFGToLLVM {
                         continue;
                     if (candidate->child1() != node)
                         continue;
-                    switch (candidate->op()) {
-                    case Int32ToDouble:
-                        int32ToDouble = candidate;
-                        break;
-                    case ValueToInt32:
-                        valueToInt32 = candidate;
-                        break;
-                    case UInt32ToNumber:
-                        uint32ToNumber = candidate;
-                        break;
-                    case DoubleAsInt32:
-                        uint32ToNumber = candidate;
-                        break;
-                    default:
-                        ASSERT(!needsOSRForwardRewiring(candidate->op()));
-                        break;
-                    }
+                    unsigned myScore = forwardRewiringSelectionScore(candidate->op());
+                    if (myScore <= bestScore)
+                        continue;
+                    bestNode = candidate;
+                    bestScore = myScore;
                 }
 
-                if (doubleAsInt32)
-                    node = doubleAsInt32;
-                else if (int32ToDouble)
-                    node = int32ToDouble;
-                else if (valueToInt32)
-                    node = valueToInt32;
-                else if (uint32ToNumber)
-                    node = uint32ToNumber;
-
-                if (isLive(node))
+                if (bestNode) {
+                    ASSERT(isLive(bestNode));
+                    node = bestNode;
                     found = true;
+                }
             }
 
             if (!found) {