Diff
Modified: tags/Safari-537.16/Source/_javascript_Core/ChangeLog (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/ChangeLog 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/ChangeLog 2012-10-26 17:30:33 UTC (rev 132674)
@@ -1,3 +1,35 @@
+2012-10-25 Filip Pizlo <[email protected]>
+
+ 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
+
+ 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 <[email protected]>
[Qt] Fix the LLInt build on Windows
Modified: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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));
Modified: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.h (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.h 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExit.h 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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: tags/Safari-537.16/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (132673 => 132674)
--- tags/Safari-537.16/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-10-26 17:30:23 UTC (rev 132673)
+++ tags/Safari-537.16/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2012-10-26 17:30:33 UTC (rev 132674)
@@ -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)
