[webkit-changes] [132701] trunk/Source/JavaScriptCore

Fri, 26 Oct 2012 15:17:37 -0700

Title: [132701] trunk/Source/_javascript_Core
Revision
132701
Author
fpi...@apple.com
Date
2012-10-26 15:18:59 -0700 (Fri, 26 Oct 2012)

Log Message

Forward OSR calculation is wrong in the presence of multiple SetLocals, or a mix of SetLocals and Phantoms
https://bugs.webkit.org/show_bug.cgi?id=100461

Reviewed by Oliver Hunt and Gavin Barraclough.

This does a couple of things. First, it removes the part of the change in r131822 that made the forward
OSR exit calculator capable of handling multiple SetLocals. That change was wrong, because it would
blindly assume that all SetLocals had the same ValueRecovery, and would ignore the possibility that if
there is no value recovery then a ForwardCheckStructure on the first SetLocal would not know how to
recover the state associated with the second SetLocal. Then, it introduces the invariant that any bytecode
op that decomposes into multiple SetLocals must first emit dead SetLocals as hints and then emit a second
set of SetLocals to actually do the setting of the locals. This means that if a ForwardCheckStructure (or
any other hoisted forward speculation) is inserted, it will always be inserted on the second set of
SetLocals (since hoisting only touches the live ones), at which point OSR will already know about the
mov hints implied by the first set of (dead) SetLocals. This gives us the behavior we wanted, namely, that
a ForwardCheckStructure applied to a variant set by a resolve_with_base-like operation can correctly do a
forward exit while also ensuring that prior to exiting we set the appropriate locals.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGOSRExit.cpp:
(JSC::DFG::OSRExit::OSRExit):
* dfg/DFGOSRExit.h:
(OSRExit):
* dfg/DFGOSRExitCompiler.cpp:
* dfg/DFGOSRExitCompiler32_64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit):
* dfg/DFGOSRExitCompiler64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::convertLastOSRExitToForward):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (132700 => 132701)


--- trunk/Source/_javascript_Core/ChangeLog	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-10-26 22:18:59 UTC (rev 132701)
@@ -1,3 +1,37 @@
+2012-10-25  Filip Pizlo  <fpi...@apple.com>
+
+        Forward OSR calculation is wrong in the presence of multiple SetLocals, or a mix of SetLocals and Phantoms
+        https://bugs.webkit.org/show_bug.cgi?id=100461
+
+        Reviewed by Oliver Hunt and Gavin Barraclough.
+
+        This does a couple of things. First, it removes the part of the change in r131822 that made the forward
+        OSR exit calculator capable of handling multiple SetLocals. That change was wrong, because it would
+        blindly assume that all SetLocals had the same ValueRecovery, and would ignore the possibility that if
+        there is no value recovery then a ForwardCheckStructure on the first SetLocal would not know how to
+        recover the state associated with the second SetLocal. Then, it introduces the invariant that any bytecode
+        op that decomposes into multiple SetLocals must first emit dead SetLocals as hints and then emit a second
+        set of SetLocals to actually do the setting of the locals. This means that if a ForwardCheckStructure (or
+        any other hoisted forward speculation) is inserted, it will always be inserted on the second set of
+        SetLocals (since hoisting only touches the live ones), at which point OSR will already know about the
+        mov hints implied by the first set of (dead) SetLocals. This gives us the behavior we wanted, namely, that
+        a ForwardCheckStructure applied to a variant set by a resolve_with_base-like operation can correctly do a
+        forward exit while also ensuring that prior to exiting we set the appropriate locals.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGOSRExit.cpp:
+        (JSC::DFG::OSRExit::OSRExit):
+        * dfg/DFGOSRExit.h:
+        (OSRExit):
+        * dfg/DFGOSRExitCompiler.cpp:
+        * dfg/DFGOSRExitCompiler32_64.cpp:
+        (JSC::DFG::OSRExitCompiler::compileExit):
+        * dfg/DFGOSRExitCompiler64.cpp:
+        (JSC::DFG::OSRExitCompiler::compileExit):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::convertLastOSRExitToForward):
+
 2012-10-26  Simon Hausmann  <simon.hausm...@digia.com>
 
         [Qt] Fix the LLInt build on Windows

Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -3108,8 +3108,16 @@
             NodeIndex base = 0;
             NodeIndex value = 0;
             if (parseResolveOperations(prediction, identifier, operations, putToBaseOperation, &base, &value)) {
+                // First create OSR hints only.
                 set(baseDst, base);
                 set(valueDst, value);
+                
+                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
+                // *after* we have already set up the values for OSR.
+                
+                // Then do the real SetLocals.
+                set(baseDst, base);
+                set(valueDst, value);
             } else {
                 addToGraph(ForceOSRExit);
                 set(baseDst, addToGraph(GarbageValue));
@@ -3128,8 +3136,16 @@
             NodeIndex base = 0;
             NodeIndex value = 0;
             if (parseResolveOperations(prediction, identifier, operations, 0, &base, &value)) {
+                // First create OSR hints only.
                 set(baseDst, base);
                 set(valueDst, value);
+                
+                // If we try to hoist structure checks into here, then we're guaranteed that they will occur
+                // *after* we have already set up the values for OSR.
+                
+                // Then do the real SetLocals.
+                set(baseDst, base);
+                set(valueDst, value);
             } else {
                 addToGraph(ForceOSRExit);
                 set(baseDst, addToGraph(GarbageValue));

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -45,9 +45,9 @@
     , m_kind(kind)
     , m_count(0)
     , m_streamIndex(streamIndex)
+    , m_lastSetOperand(jit->m_lastSetOperand)
 {
     ASSERT(m_codeOrigin.isSet());
-    m_setOperands.append(jit->m_lastSetOperand);
 }
 
 bool OSRExit::considerAddingAsFrequentExitSiteSlow(CodeBlock* dfgCodeBlock, CodeBlock* profiledCodeBlock)

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExit.h (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExit.h	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExit.h	2012-10-26 22:18:59 UTC (rev 132701)
@@ -110,9 +110,9 @@
     }
     
     unsigned m_streamIndex;
-    Vector<int, 1> m_setOperands;
+    int m_lastSetOperand;
     
-    Vector<RefPtr<ValueRecoveryOverride>, 1> m_valueRecoveryOverrides;
+    RefPtr<ValueRecoveryOverride> m_valueRecoveryOverride;
 
 private:
     bool considerAddingAsFrequentExitSiteSlow(CodeBlock* dfgCodeBlock, CodeBlock* profiledCodeBlock);

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -70,10 +70,11 @@
     Operands<ValueRecovery> operands;
     codeBlock->variableEventStream().reconstruct(codeBlock, exit.m_codeOrigin, codeBlock->minifiedDFG(), exit.m_streamIndex, operands);
     
-    // There may be overrides, for forward speculations.
-    for (size_t i = 0; i < exit.m_valueRecoveryOverrides.size(); i++)
-        operands.setOperand(exit.m_valueRecoveryOverrides[i]->operand, exit.m_valueRecoveryOverrides[i]->recovery);
-
+    // There may be an override, for forward speculations.
+    if (!!exit.m_valueRecoveryOverride) {
+        operands.setOperand(
+            exit.m_valueRecoveryOverride->operand, exit.m_valueRecoveryOverride->recovery);
+    }
     
     SpeculationRecovery* recovery = 0;
     if (exit.m_recoveryIndex)

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -732,9 +732,9 @@
     
     // 15) Load the result of the last bytecode operation into regT0.
     
-    for (size_t i = 0; i < exit.m_setOperands.size(); i++) {
-        m_jit.load32(AssemblyHelpers::payloadFor((VirtualRegister)exit.m_setOperands[i]), GPRInfo::cachedResultRegister);
-        m_jit.load32(AssemblyHelpers::tagFor((VirtualRegister)exit.m_setOperands[i]), GPRInfo::cachedResultRegister2);
+    if (exit.m_lastSetOperand != std::numeric_limits<int>::max()) {
+        m_jit.load32(AssemblyHelpers::payloadFor((VirtualRegister)exit.m_lastSetOperand), GPRInfo::cachedResultRegister);
+        m_jit.load32(AssemblyHelpers::tagFor((VirtualRegister)exit.m_lastSetOperand), GPRInfo::cachedResultRegister2);
     }
     
     // 16) Adjust the call frame pointer.

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -681,9 +681,9 @@
     
     // 16) Load the result of the last bytecode operation into regT0.
     
-    for (size_t i = 0; i < exit.m_setOperands.size(); i++)
-        m_jit.load64(AssemblyHelpers::addressFor((VirtualRegister)exit.m_setOperands[i]), GPRInfo::cachedResultRegister);
-
+    if (exit.m_lastSetOperand != std::numeric_limits<int>::max())
+        m_jit.loadPtr(AssemblyHelpers::addressFor((VirtualRegister)exit.m_lastSetOperand), GPRInfo::cachedResultRegister);
+    
     // 17) Adjust the call frame pointer.
     
     if (exit.m_codeOrigin.inlineCallFrame)

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (132700 => 132701)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-10-26 21:59:55 UTC (rev 132700)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp	2012-10-26 22:18:59 UTC (rev 132701)
@@ -153,19 +153,37 @@
 
 void SpeculativeJIT::convertLastOSRExitToForward(const ValueRecovery& valueRecovery)
 {
-#if !ASSERT_DISABLED
     if (!valueRecovery) {
         // Check that the preceding node was a SetLocal with the same code origin.
         Node* setLocal = &at(m_jit.graph().m_blocks[m_block]->at(m_indexInBlock - 1));
-        ASSERT(setLocal->op() == SetLocal);
-        ASSERT(setLocal->codeOrigin == at(m_compileIndex).codeOrigin);
+        ASSERT_UNUSED(setLocal, setLocal->op() == SetLocal);
+        ASSERT_UNUSED(setLocal, setLocal->codeOrigin == at(m_compileIndex).codeOrigin);
+        
+        // Find the next node.
+        unsigned indexInBlock = m_indexInBlock + 1;
+        Node* node = 0;
+        for (;;) {
+            if (indexInBlock == m_jit.graph().m_blocks[m_block]->size()) {
+                // This is an inline return. Give up and do a backwards speculation. This is safe
+                // because an inline return has its own bytecode index and it's always safe to
+                // reexecute that bytecode.
+                ASSERT(node->op() == Jump);
+                return;
+            }
+            node = &at(m_jit.graph().m_blocks[m_block]->at(indexInBlock));
+            if (node->codeOrigin != at(m_compileIndex).codeOrigin)
+                break;
+            indexInBlock++;
+        }
+        
+        ASSERT(node->codeOrigin != at(m_compileIndex).codeOrigin);
+        OSRExit& exit = m_jit.codeBlock()->lastOSRExit();
+        exit.m_codeOrigin = node->codeOrigin;
+        return;
     }
-#endif
     
     unsigned setLocalIndexInBlock = m_indexInBlock + 1;
-
-    OSRExit& exit = m_jit.codeBlock()->lastOSRExit();
-
+    
     Node* setLocal = &at(m_jit.graph().m_blocks[m_block]->at(setLocalIndexInBlock));
     bool hadInt32ToDouble = false;
     
@@ -175,13 +193,11 @@
     }
     if (setLocal->op() == Flush || setLocal->op() == Phantom)
         setLocal = &at(m_jit.graph().m_blocks[m_block]->at(++setLocalIndexInBlock));
-    
-    if (!!valueRecovery) {
-        if (hadInt32ToDouble)
-            ASSERT(at(setLocal->child1()).child1() == m_compileIndex);
-        else
-            ASSERT(setLocal->child1() == m_compileIndex);
-    }
+        
+    if (hadInt32ToDouble)
+        ASSERT(at(setLocal->child1()).child1() == m_compileIndex);
+    else
+        ASSERT(setLocal->child1() == m_compileIndex);
     ASSERT(setLocal->op() == SetLocal);
     ASSERT(setLocal->codeOrigin == at(m_compileIndex).codeOrigin);
 
@@ -190,34 +206,14 @@
         // We're at an inlined return. Use a backward speculation instead.
         return;
     }
-
-    exit.m_setOperands[0] = setLocal->local();
-    while (nextNode->codeOrigin == at(m_compileIndex).codeOrigin) {
-        ++setLocalIndexInBlock;
-        Node* nextSetLocal = nextNode;
-        if (nextSetLocal->op() == Int32ToDouble)
-            nextSetLocal = &at(m_jit.graph().m_blocks[m_block]->at(++setLocalIndexInBlock));
-
-        if (nextSetLocal->op() == Flush || nextSetLocal->op() == Phantom)
-            nextSetLocal = &at(m_jit.graph().m_blocks[m_block]->at(++setLocalIndexInBlock));
-
-        nextNode = &at(m_jit.graph().m_blocks[m_block]->at(setLocalIndexInBlock + 1));
-        ASSERT(nextNode->op() != Jump || nextNode->codeOrigin != at(m_compileIndex).codeOrigin);
-        ASSERT(nextSetLocal->op() == SetLocal);
-        exit.m_setOperands.append(nextSetLocal->local());
-    }
-
     ASSERT(nextNode->codeOrigin != at(m_compileIndex).codeOrigin);
-
+        
+    OSRExit& exit = m_jit.codeBlock()->lastOSRExit();
     exit.m_codeOrigin = nextNode->codeOrigin;
         
-    if (!valueRecovery)
-        return;
-
-    ASSERT(exit.m_setOperands.size() == 1);
-    for (size_t i = 0; i < exit.m_setOperands.size(); i++)
-        exit.m_valueRecoveryOverrides.append(adoptRef(new ValueRecoveryOverride(exit.m_setOperands[i], valueRecovery)));
-
+    exit.m_lastSetOperand = setLocal->local();
+    exit.m_valueRecoveryOverride = adoptRef(
+        new ValueRecoveryOverride(setLocal->local(), valueRecovery));
 }
 
 JumpReplacementWatchpoint* SpeculativeJIT::forwardSpeculationWatchpoint(ExitKind kind)

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to