Changeset 132701 in webkit

Timestamp:

Oct 26, 2012, 3:18:59 PM (13 years ago)

Author:

fpizlo@apple.com

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGByteCodeParser.cpp (modified) (2 diffs)
• dfg/DFGOSRExit.cpp (modified) (1 diff)
• dfg/DFGOSRExit.h (modified) (1 diff)
• dfg/DFGOSRExitCompiler.cpp (modified) (1 diff)
• dfg/DFGOSRExitCompiler32_64.cpp (modified) (1 diff)
• dfg/DFGOSRExitCompiler64.cpp (modified) (1 diff)
• dfg/DFGSpeculativeJIT.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2012-10-25  Filip Pizlo  <fpizlo@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.hausmann@digia.com>
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -3109 +3109 @@
             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);
@@ -3129 +3137 @@
             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);

trunk/Source/JavaScriptCore/dfg/DFGOSRExit.cpp

@@ -46 +46 @@
     , m_count(0)
     , m_streamIndex(streamIndex)
+    , m_lastSetOperand(jit->m_lastSetOperand)
 {
     ASSERT(m_codeOrigin.isSet());
-    m_setOperands.append(jit->m_lastSetOperand);
 }
 

trunk/Source/JavaScriptCore/dfg/DFGOSRExit.h

@@ -111 +111 @@
     
     unsigned m_streamIndex;
-    Vector<int, 1> m_setOperands;
+    int m_lastSetOperand;
     
-    Vector<RefPtr<ValueRecoveryOverride>, 1> m_valueRecoveryOverrides;
+    RefPtr<ValueRecoveryOverride> m_valueRecoveryOverride;
 
 private:

trunk/Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp

@@ -71 +71 @@
     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;

trunk/Source/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp

@@ -733 +733 @@
     // 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);
     }
     

trunk/Source/JavaScriptCore/dfg/DFGOSRExitCompiler64.cpp

@@ -682 +682 @@
     // 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.
     

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -154 +154 @@
 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);
-    }
-#endif
+        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;
+    }
     
     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;
@@ -176 +194 @@
     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);
@@ -191 +207 @@
         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));
 }