- Revision
- 129089
- Author
- [email protected]
- Date
- 2012-09-19 21:44:43 -0700 (Wed, 19 Sep 2012)
Log Message
OSR exit sometimes neglects to create the arguments object
https://bugs.webkit.org/show_bug.cgi?id=97162
Reviewed by Filip Pizlo.
../_javascript_Core:
No performance change.
I don't know of any case where this is a real problem in TOT, but it
will become a problem if we start compiling eval, with, or catch, and/or
sometimes stop doing arguments optimizations in the bytecode.
* dfg/DFGArgumentsSimplificationPhase.cpp:
(JSC::DFG::ArgumentsSimplificationPhase::run): Account for a
CreateArguments that has transformed into PhantomArguments. We used to
clear our reference to the CreateArguments node, but now we hold onto it,
so we need to account for it transforming.
Don't replace a SetLocal(CreateArguments) with a SetLocal(JSValue())
because that doesn't leave enough information behind for OSR exit to do
the right thing. Instead, maintain our reference to CreateArguments, and
rely on CreateArguments transforming into PhantomArguments after
optimization. SetLocal(PhantomArguments) is efficient, and it's a marker
for OSR exit to create the arguments object.
Don't ASSERT that all PhantomArguments are unreferenced because we now
leave them in the graph as SetLocal(PhantomArguments), and that's harmless.
* dfg/DFGArgumentsSimplificationPhase.h:
(NullableHashTraits):
(JSC::DFG::NullableHashTraits::emptyValue): Export our special hash table
for inline call frames so the OSR exit compiler can use it.
* dfg/DFGOSRExitCompiler32_64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit):
* dfg/DFGOSRExitCompiler64.cpp:
(JSC::DFG::OSRExitCompiler::compileExit): Don't load the 'arguments'
register to decide if we need to create the arguments object. Optimization
may have eliminated the initializing store to this register, in which
case we'll load garbage. Instead, use the global knowledge that all call
frames that optimized out 'arguments' now need to create it, and use a hash
table to make sure we do so only once per call frame.
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile): SetLocal(PhantomArguments) is unique
because we haven't just changed a value's format or elided a load or store;
instead, we've replaced an object with JSValue(). We could try to account
for this in a general way, but for now it's a special-case optimization,
so we give it a specific OSR hint instead.
../WTF:
* wtf/HashTraits.h:
(NullableHashTraits):
(WTF::NullableHashTraits::emptyValue):
(WTF):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (129088 => 129089)
--- trunk/Source/_javascript_Core/ChangeLog 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-09-20 04:44:43 UTC (rev 129089)
@@ -1,3 +1,54 @@
+2012-09-19 Geoffrey Garen <[email protected]>
+
+ OSR exit sometimes neglects to create the arguments object
+ https://bugs.webkit.org/show_bug.cgi?id=97162
+
+ Reviewed by Filip Pizlo.
+
+ No performance change.
+
+ I don't know of any case where this is a real problem in TOT, but it
+ will become a problem if we start compiling eval, with, or catch, and/or
+ sometimes stop doing arguments optimizations in the bytecode.
+
+ * dfg/DFGArgumentsSimplificationPhase.cpp:
+ (JSC::DFG::ArgumentsSimplificationPhase::run): Account for a
+ CreateArguments that has transformed into PhantomArguments. We used to
+ clear our reference to the CreateArguments node, but now we hold onto it,
+ so we need to account for it transforming.
+
+ Don't replace a SetLocal(CreateArguments) with a SetLocal(JSValue())
+ because that doesn't leave enough information behind for OSR exit to do
+ the right thing. Instead, maintain our reference to CreateArguments, and
+ rely on CreateArguments transforming into PhantomArguments after
+ optimization. SetLocal(PhantomArguments) is efficient, and it's a marker
+ for OSR exit to create the arguments object.
+
+ Don't ASSERT that all PhantomArguments are unreferenced because we now
+ leave them in the graph as SetLocal(PhantomArguments), and that's harmless.
+
+ * dfg/DFGArgumentsSimplificationPhase.h:
+ (NullableHashTraits):
+ (JSC::DFG::NullableHashTraits::emptyValue): Export our special hash table
+ for inline call frames so the OSR exit compiler can use it.
+
+ * dfg/DFGOSRExitCompiler32_64.cpp:
+ (JSC::DFG::OSRExitCompiler::compileExit):
+ * dfg/DFGOSRExitCompiler64.cpp:
+ (JSC::DFG::OSRExitCompiler::compileExit): Don't load the 'arguments'
+ register to decide if we need to create the arguments object. Optimization
+ may have eliminated the initializing store to this register, in which
+ case we'll load garbage. Instead, use the global knowledge that all call
+ frames that optimized out 'arguments' now need to create it, and use a hash
+ table to make sure we do so only once per call frame.
+
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile): SetLocal(PhantomArguments) is unique
+ because we haven't just changed a value's format or elided a load or store;
+ instead, we've replaced an object with JSValue(). We could try to account
+ for this in a general way, but for now it's a special-case optimization,
+ so we give it a specific OSR hint instead.
+
2012-09-19 Filip Pizlo <[email protected]>
REGRESSION(r128802): It made some JS tests crash
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsSimplificationPhase.cpp (129088 => 129089)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsSimplificationPhase.cpp 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsSimplificationPhase.cpp 2012-09-20 04:44:43 UTC (rev 129089)
@@ -41,12 +41,6 @@
namespace {
-template<typename T>
-struct NullableHashTraits : public HashTraits<T> {
- static const bool emptyValueIsZero = false;
- static T emptyValue() { return reinterpret_cast<T>(1); }
-};
-
struct ArgumentsAliasingData {
InlineCallFrame* callContext;
bool callContextSet;
@@ -181,7 +175,7 @@
VariableAccessData* variableAccessData = node.variableAccessData();
int argumentsRegister =
m_graph.uncheckedArgumentsRegisterFor(node.codeOrigin);
- if (source.op() != CreateArguments) {
+ if (source.op() != CreateArguments && source.op() != PhantomArguments) {
// Make sure that the source of the SetLocal knows that if it's
// a variable that we think is aliased to the arguments, then it
// may escape at this point. In future, we could track transitive
@@ -435,18 +429,9 @@
VariableAccessData* variableAccessData = node.variableAccessData();
if (m_graph.argumentsRegisterFor(node.codeOrigin) == variableAccessData->local()
- || unmodifiedArgumentsRegister(m_graph.argumentsRegisterFor(node.codeOrigin)) == variableAccessData->local()) {
- // The child of this store should really be the empty value.
- Node emptyJSValue(JSConstant, node.codeOrigin, OpInfo(codeBlock()->addOrFindConstant(JSValue())));
- emptyJSValue.ref();
- NodeIndex emptyJSValueIndex = m_graph.size();
- m_graph.deref(node.child1());
- node.children.child1() = Edge(emptyJSValueIndex);
- m_graph.append(emptyJSValue);
- insertionSet.append(indexInBlock, emptyJSValueIndex);
- changed = true;
+ || unmodifiedArgumentsRegister(m_graph.argumentsRegisterFor(node.codeOrigin)) == variableAccessData->local())
break;
- }
+
ASSERT(!variableAccessData->isCaptured());
// If this is a store into a VariableAccessData* that is marked as
@@ -661,25 +646,8 @@
insertionSet.execute(*block);
}
- if (changed) {
+ if (changed)
m_graph.collectGarbage();
-
- // Verify that PhantomArguments nodes are not shouldGenerate().
-#if !ASSERT_DISABLED
- for (BlockIndex blockIndex = 0; blockIndex < m_graph.m_blocks.size(); ++blockIndex) {
- BasicBlock* block = m_graph.m_blocks[blockIndex].get();
- if (!block)
- continue;
- for (unsigned indexInBlock = 0; indexInBlock < block->size(); ++indexInBlock) {
- NodeIndex nodeIndex = block->at(indexInBlock);
- Node& node = m_graph[nodeIndex];
- if (node.op() != PhantomArguments)
- continue;
- ASSERT(!node.shouldGenerate());
- }
- }
-#endif
- }
return changed;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp (129088 => 129089)
--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler32_64.cpp 2012-09-20 04:44:43 UTC (rev 129089)
@@ -612,6 +612,9 @@
// registers.
if (haveArguments) {
+ HashSet<InlineCallFrame*, DefaultHash<InlineCallFrame*>::Hash,
+ NullableHashTraits<InlineCallFrame*> > didCreateArgumentsObject;
+
for (size_t index = 0; index < operands.size(); ++index) {
const ValueRecovery& recovery = operands[index];
if (recovery.technique() != ArgumentsThatWereNotCreated)
@@ -627,46 +630,44 @@
break;
}
}
+
int argumentsRegister = m_jit.argumentsRegisterFor(inlineCallFrame);
-
+ if (didCreateArgumentsObject.add(inlineCallFrame).isNewEntry) {
+ // We know this call frame optimized out an arguments object that
+ // the baseline JIT would have created. Do that creation now.
+ if (inlineCallFrame) {
+ m_jit.setupArgumentsWithExecState(
+ AssemblyHelpers::TrustedImmPtr(inlineCallFrame));
+ m_jit.move(
+ AssemblyHelpers::TrustedImmPtr(
+ bitwise_cast<void*>(operationCreateInlinedArguments)),
+ GPRInfo::nonArgGPR0);
+ } else {
+ m_jit.setupArgumentsExecState();
+ m_jit.move(
+ AssemblyHelpers::TrustedImmPtr(
+ bitwise_cast<void*>(operationCreateArguments)),
+ GPRInfo::nonArgGPR0);
+ }
+ m_jit.call(GPRInfo::nonArgGPR0);
+ m_jit.store32(
+ AssemblyHelpers::TrustedImm32(JSValue::CellTag),
+ AssemblyHelpers::tagFor(argumentsRegister));
+ m_jit.store32(
+ GPRInfo::returnValueGPR,
+ AssemblyHelpers::payloadFor(argumentsRegister));
+ m_jit.store32(
+ AssemblyHelpers::TrustedImm32(JSValue::CellTag),
+ AssemblyHelpers::tagFor(unmodifiedArgumentsRegister(argumentsRegister)));
+ m_jit.store32(
+ GPRInfo::returnValueGPR,
+ AssemblyHelpers::payloadFor(unmodifiedArgumentsRegister(argumentsRegister)));
+ m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
+ }
+
m_jit.load32(AssemblyHelpers::payloadFor(argumentsRegister), GPRInfo::regT0);
- AssemblyHelpers::Jump haveArguments = m_jit.branch32(
- AssemblyHelpers::NotEqual,
- AssemblyHelpers::tagFor(argumentsRegister),
- AssemblyHelpers::TrustedImm32(JSValue::EmptyValueTag));
-
- if (inlineCallFrame) {
- m_jit.setupArgumentsWithExecState(
- AssemblyHelpers::TrustedImmPtr(inlineCallFrame));
- m_jit.move(
- AssemblyHelpers::TrustedImmPtr(
- bitwise_cast<void*>(operationCreateInlinedArguments)),
- GPRInfo::nonArgGPR0);
- } else {
- m_jit.setupArgumentsExecState();
- m_jit.move(
- AssemblyHelpers::TrustedImmPtr(
- bitwise_cast<void*>(operationCreateArguments)),
- GPRInfo::nonArgGPR0);
- }
- m_jit.call(GPRInfo::nonArgGPR0);
m_jit.store32(
AssemblyHelpers::TrustedImm32(JSValue::CellTag),
- AssemblyHelpers::tagFor(argumentsRegister));
- m_jit.store32(
- GPRInfo::returnValueGPR,
- AssemblyHelpers::payloadFor(argumentsRegister));
- m_jit.store32(
- AssemblyHelpers::TrustedImm32(JSValue::CellTag),
- AssemblyHelpers::tagFor(unmodifiedArgumentsRegister(argumentsRegister)));
- m_jit.store32(
- GPRInfo::returnValueGPR,
- AssemblyHelpers::payloadFor(unmodifiedArgumentsRegister(argumentsRegister)));
- m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
-
- haveArguments.link(&m_jit);
- m_jit.store32(
- AssemblyHelpers::TrustedImm32(JSValue::CellTag),
AssemblyHelpers::tagFor(operand));
m_jit.store32(GPRInfo::regT0, AssemblyHelpers::payloadFor(operand));
}
Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp (129088 => 129089)
--- trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExitCompiler64.cpp 2012-09-20 04:44:43 UTC (rev 129089)
@@ -587,6 +587,9 @@
// registers.
if (haveArguments) {
+ HashSet<InlineCallFrame*, DefaultHash<InlineCallFrame*>::Hash,
+ NullableHashTraits<InlineCallFrame*> > didCreateArgumentsObject;
+
for (size_t index = 0; index < operands.size(); ++index) {
const ValueRecovery& recovery = operands[index];
if (recovery.technique() != ArgumentsThatWereNotCreated)
@@ -602,29 +605,29 @@
break;
}
}
+
int argumentsRegister = m_jit.argumentsRegisterFor(inlineCallFrame);
-
+ if (didCreateArgumentsObject.add(inlineCallFrame).isNewEntry) {
+ // We know this call frame optimized out an arguments object that
+ // the baseline JIT would have created. Do that creation now.
+ if (inlineCallFrame) {
+ m_jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->stackOffset * sizeof(EncodedJSValue)), GPRInfo::callFrameRegister, GPRInfo::regT0);
+ m_jit.setupArguments(GPRInfo::regT0);
+ } else
+ m_jit.setupArgumentsExecState();
+ m_jit.move(
+ AssemblyHelpers::TrustedImmPtr(
+ bitwise_cast<void*>(operationCreateArguments)),
+ GPRInfo::nonArgGPR0);
+ m_jit.call(GPRInfo::nonArgGPR0);
+ m_jit.storePtr(GPRInfo::returnValueGPR, AssemblyHelpers::addressFor(argumentsRegister));
+ m_jit.storePtr(
+ GPRInfo::returnValueGPR,
+ AssemblyHelpers::addressFor(unmodifiedArgumentsRegister(argumentsRegister)));
+ m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
+ }
+
m_jit.loadPtr(AssemblyHelpers::addressFor(argumentsRegister), GPRInfo::regT0);
- AssemblyHelpers::Jump haveArguments = m_jit.branchTestPtr(
- AssemblyHelpers::NonZero, GPRInfo::regT0);
-
- if (inlineCallFrame) {
- m_jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->stackOffset * sizeof(EncodedJSValue)), GPRInfo::callFrameRegister, GPRInfo::regT0);
- m_jit.setupArguments(GPRInfo::regT0);
- } else
- m_jit.setupArgumentsExecState();
- m_jit.move(
- AssemblyHelpers::TrustedImmPtr(
- bitwise_cast<void*>(operationCreateArguments)),
- GPRInfo::nonArgGPR0);
- m_jit.call(GPRInfo::nonArgGPR0);
- m_jit.storePtr(GPRInfo::returnValueGPR, AssemblyHelpers::addressFor(argumentsRegister));
- m_jit.storePtr(
- GPRInfo::returnValueGPR,
- AssemblyHelpers::addressFor(unmodifiedArgumentsRegister(argumentsRegister)));
- m_jit.move(GPRInfo::returnValueGPR, GPRInfo::regT0); // no-op move on almost all platforms.
-
- haveArguments.link(&m_jit);
m_jit.storePtr(GPRInfo::regT0, AssemblyHelpers::addressFor(operand));
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (129088 => 129089)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2012-09-20 04:44:43 UTC (rev 129089)
@@ -2055,9 +2055,6 @@
break;
case PhantomArguments:
- // This should never be must-generate.
- ASSERT_NOT_REACHED();
- // But as a release-mode fall-back make it the empty value.
initConstantInfo(m_compileIndex);
break;
@@ -2244,6 +2241,15 @@
m_jit.store32(value.tagGPR(), JITCompiler::tagFor(node.local()));
noResult(m_compileIndex);
recordSetLocal(node.local(), ValueSource(ValueInRegisterFile));
+
+ // If we're storing an arguments object that has been optimized away,
+ // our variable event stream for OSR exit now reflects the optimized
+ // value (JSValue()). On the slow path, we want an arguments object
+ // instead. We add an additional move hint to show OSR exit that it
+ // needs to reconstruct the arguments object.
+ if (at(node.child1()).op() == PhantomArguments)
+ compileMovHint(node);
+
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (129088 => 129089)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2012-09-20 04:44:43 UTC (rev 129089)
@@ -2119,9 +2119,6 @@
break;
case PhantomArguments:
- // This should never be must-generate.
- ASSERT_NOT_REACHED();
- // But as a release-mode fall-back make it the empty value.
initConstantInfo(m_compileIndex);
break;
@@ -2280,6 +2277,15 @@
noResult(m_compileIndex);
recordSetLocal(node.local(), ValueSource(ValueInRegisterFile));
+
+ // If we're storing an arguments object that has been optimized away,
+ // our variable event stream for OSR exit now reflects the optimized
+ // value (JSValue()). On the slow path, we want an arguments object
+ // instead. We add an additional move hint to show OSR exit that it
+ // needs to reconstruct the arguments object.
+ if (at(node.child1()).op() == PhantomArguments)
+ compileMovHint(node);
+
break;
}
Modified: trunk/Source/WTF/ChangeLog (129088 => 129089)
--- trunk/Source/WTF/ChangeLog 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/WTF/ChangeLog 2012-09-20 04:44:43 UTC (rev 129089)
@@ -1,3 +1,15 @@
+2012-09-19 Geoffrey Garen <[email protected]>
+
+ OSR exit sometimes neglects to create the arguments object
+ https://bugs.webkit.org/show_bug.cgi?id=97162
+
+ Reviewed by Filip Pizlo.
+
+ * wtf/HashTraits.h:
+ (NullableHashTraits):
+ (WTF::NullableHashTraits::emptyValue):
+ (WTF):
+
2012-09-18 Glenn Adams <[email protected]>
WTFString::show doesn't dump non-ASCII characters in a readable manner
Modified: trunk/Source/WTF/wtf/HashTraits.h (129088 => 129089)
--- trunk/Source/WTF/wtf/HashTraits.h 2012-09-20 04:27:46 UTC (rev 129088)
+++ trunk/Source/WTF/wtf/HashTraits.h 2012-09-20 04:44:43 UTC (rev 129089)
@@ -231,9 +231,16 @@
template<typename Key, typename Value>
struct HashTraits<KeyValuePair<Key, Value> > : public KeyValuePairHashTraits<HashTraits<Key>, HashTraits<Value> > { };
+ template<typename T>
+ struct NullableHashTraits : public HashTraits<T> {
+ static const bool emptyValueIsZero = false;
+ static T emptyValue() { return reinterpret_cast<T>(1); }
+ };
+
} // namespace WTF
using WTF::HashTraits;
using WTF::PairHashTraits;
+using WTF::NullableHashTraits;
#endif // WTF_HashTraits_h
