Title: [171190] trunk/Source/_javascript_Core
- Revision
- 171190
- Author
- [email protected]
- Date
- 2014-07-17 11:17:43 -0700 (Thu, 17 Jul 2014)
Log Message
DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
https://bugs.webkit.org/show_bug.cgi?id=134988
<rdar://problem/17706349>
Reviewed by Oliver Hunt.
Luckily, we also don't need this optimization to be super powerful: the only place
where it really matters is for getting rid of the redundancy between op_enter and
op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
two things. This change updates the store eliminator to know about only that small,
obviously safe, set of nodes over which we can store-eliminate.
This shouldn't have any performance impact in the DFG because this optimization kicks
in relatively rarely already. And once we tier up into the FTL, we get a much better
store elimination over LLVM IR, so this really shouldn't matter at all.
The tricky part of this patch is that there is a close relative of this optimization,
for uncaptured variables that got flushed. This happens for arguments to inlined calls.
I make this work by splitting it into two different store eliminators.
Note that in the process of crafting the tests, I realized that we were incorrectly
DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
to a check node followed by the actual allocation.
* dfg/DFGCSEPhase.cpp:
(JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::setLocalStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE):
(JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.
* dfg/DFGNodeType.h:
* tests/stress/capture-escape-and-throw.js: Added.
(foo.f):
(foo):
* tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.
(foo):
(bar):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (171189 => 171190)
--- trunk/Source/_javascript_Core/ChangeLog 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-07-17 18:17:43 UTC (rev 171190)
@@ -1,3 +1,44 @@
+2014-07-16 Filip Pizlo <[email protected]>
+
+ DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
+ https://bugs.webkit.org/show_bug.cgi?id=134988
+ <rdar://problem/17706349>
+
+ Reviewed by Oliver Hunt.
+
+ Luckily, we also don't need this optimization to be super powerful: the only place
+ where it really matters is for getting rid of the redundancy between op_enter and
+ op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
+ two things. This change updates the store eliminator to know about only that small,
+ obviously safe, set of nodes over which we can store-eliminate.
+
+ This shouldn't have any performance impact in the DFG because this optimization kicks
+ in relatively rarely already. And once we tier up into the FTL, we get a much better
+ store elimination over LLVM IR, so this really shouldn't matter at all.
+
+ The tricky part of this patch is that there is a close relative of this optimization,
+ for uncaptured variables that got flushed. This happens for arguments to inlined calls.
+ I make this work by splitting it into two different store eliminators.
+
+ Note that in the process of crafting the tests, I realized that we were incorrectly
+ DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
+ negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
+ to a check node followed by the actual allocation.
+
+ * dfg/DFGCSEPhase.cpp:
+ (JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
+ (JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
+ (JSC::DFG::CSEPhase::setLocalStoreElimination):
+ (JSC::DFG::CSEPhase::performNodeCSE):
+ (JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.
+ * dfg/DFGNodeType.h:
+ * tests/stress/capture-escape-and-throw.js: Added.
+ (foo.f):
+ (foo):
+ * tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.
+ (foo):
+ (bar):
+
2014-07-15 Benjamin Poulain <[email protected]>
Reduce the overhead of updating the AssemblerBuffer
Modified: trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp (171189 => 171190)
--- trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2014-07-17 18:17:43 UTC (rev 171190)
@@ -928,48 +928,34 @@
return 0;
}
- struct SetLocalStoreEliminationResult {
- SetLocalStoreEliminationResult()
- : mayBeAccessed(false)
- , mayExit(false)
- , mayClobberWorld(false)
- {
- }
-
- bool mayBeAccessed;
- bool mayExit;
- bool mayClobberWorld;
- };
- SetLocalStoreEliminationResult setLocalStoreElimination(
- VirtualRegister local, Node* expectedNode)
+ bool uncapturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
{
- SetLocalStoreEliminationResult result;
for (unsigned i = m_indexInBlock; i--;) {
Node* node = m_currentBlock->at(i);
switch (node->op()) {
case GetLocal:
case Flush:
if (node->local() == local)
- result.mayBeAccessed = true;
+ return false;
break;
case GetLocalUnlinked:
if (node->unlinkedLocal() == local)
- result.mayBeAccessed = true;
+ return false;
break;
case SetLocal: {
if (node->local() != local)
break;
if (node != expectedNode)
- result.mayBeAccessed = true;
- return result;
+ return false;
+ return true;
}
case GetClosureVar:
case PutClosureVar:
if (static_cast<VirtualRegister>(node->varNumber()) == local)
- result.mayBeAccessed = true;
+ return false;
break;
case GetMyScope:
@@ -977,25 +963,24 @@
if (node->origin.semantic.inlineCallFrame)
break;
if (m_graph.uncheckedActivationRegister() == local)
- result.mayBeAccessed = true;
+ return false;
break;
case CheckArgumentsNotCreated:
case GetMyArgumentsLength:
case GetMyArgumentsLengthSafe:
if (m_graph.uncheckedArgumentsRegisterFor(node->origin.semantic) == local)
- result.mayBeAccessed = true;
+ return false;
break;
case GetMyArgumentByVal:
case GetMyArgumentByValSafe:
- result.mayBeAccessed = true;
- break;
+ return false;
case GetByVal:
// If this is accessing arguments then it's potentially accessing locals.
if (node->arrayMode().type() == Array::Arguments)
- result.mayBeAccessed = true;
+ return false;
break;
case CreateArguments:
@@ -1005,21 +990,66 @@
// are live. We could be clever here and check if the local qualifies as an
// argument register. But that seems like it would buy us very little since
// any kind of tear offs are rare to begin with.
- result.mayBeAccessed = true;
- break;
+ return false;
default:
break;
}
- result.mayExit |= node->canExit();
- result.mayClobberWorld |= m_graph.clobbersWorld(node);
+ if (m_graph.clobbersWorld(node))
+ return false;
}
RELEASE_ASSERT_NOT_REACHED();
- // Be safe in release mode.
- result.mayBeAccessed = true;
- return result;
+ return false;
}
+
+ bool capturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
+ {
+ for (unsigned i = m_indexInBlock; i--;) {
+ Node* node = m_currentBlock->at(i);
+ switch (node->op()) {
+ case GetLocal:
+ case Flush:
+ if (node->local() == local)
+ return false;
+ break;
+
+ case GetLocalUnlinked:
+ if (node->unlinkedLocal() == local)
+ return false;
+ break;
+
+ case SetLocal: {
+ if (node->local() != local)
+ break;
+ if (node != expectedNode)
+ return false;
+ return true;
+ }
+
+ case Phantom:
+ case Check:
+ case HardPhantom:
+ case MovHint:
+ case JSConstant:
+ case DoubleConstant:
+ case Int52Constant:
+ break;
+
+ default:
+ return false;
+ }
+ }
+ RELEASE_ASSERT_NOT_REACHED();
+ return false;
+ }
+ bool setLocalStoreElimination(VariableAccessData* variableAccessData, Node* expectedNode)
+ {
+ if (variableAccessData->isCaptured())
+ return capturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
+ return uncapturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
+ }
+
bool invalidationPointElimination()
{
for (unsigned i = m_indexInBlock; i--;) {
@@ -1215,7 +1245,6 @@
case Flush: {
ASSERT(m_graph.m_form != SSA);
VariableAccessData* variableAccessData = node->variableAccessData();
- VirtualRegister local = variableAccessData->local();
if (!node->child1()) {
// FIXME: It's silly that we punt on flush-eliminating here. We don't really
// need child1 to figure out what's going on.
@@ -1239,12 +1268,9 @@
if (replacement->canExit())
break;
}
- SetLocalStoreEliminationResult result =
- setLocalStoreElimination(local, replacement);
- if (result.mayBeAccessed || result.mayClobberWorld)
+ if (!setLocalStoreElimination(variableAccessData, replacement))
break;
ASSERT(replacement->op() == SetLocal);
- // FIXME: Investigate using mayExit as a further optimization.
node->convertToPhantom();
Node* dataNode = replacement->child1().node();
ASSERT(dataNode->hasResult());
Modified: trunk/Source/_javascript_Core/dfg/DFGNodeType.h (171189 => 171190)
--- trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2014-07-17 17:43:31 UTC (rev 171189)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2014-07-17 18:17:43 UTC (rev 171190)
@@ -233,7 +233,7 @@
/* Allocations. */\
macro(NewObject, NodeResultJS) \
macro(NewArray, NodeResultJS | NodeHasVarArgs) \
- macro(NewArrayWithSize, NodeResultJS) \
+ macro(NewArrayWithSize, NodeResultJS | NodeMustGenerate) \
macro(NewArrayBuffer, NodeResultJS) \
macro(NewTypedArray, NodeResultJS | NodeClobbersWorld | NodeMustGenerate) \
macro(NewRegexp, NodeResultJS) \
Added: trunk/Source/_javascript_Core/tests/stress/capture-escape-and-throw.js (0 => 171190)
--- trunk/Source/_javascript_Core/tests/stress/capture-escape-and-throw.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/capture-escape-and-throw.js 2014-07-17 18:17:43 UTC (rev 171190)
@@ -0,0 +1,28 @@
+var f;
+
+function foo(s) {
+ var x = 1;
+ f = function() { return x; };
+ x = 2;
+ new Array(s);
+ x = 3;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i)
+ foo(1);
+
+var didThrow = false;
+try {
+ foo(-1);
+} catch (e) {
+ didThrow = e;
+}
+
+if (("" + didThrow).indexOf("RangeError") != 0)
+ throw "Error: did not throw right exception: " + didThrow;
+
+var result = f();
+if (result != 2)
+ throw "Error: bad result from f(): " + result;
Added: trunk/Source/_javascript_Core/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js (0 => 171190)
--- trunk/Source/_javascript_Core/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js 2014-07-17 18:17:43 UTC (rev 171190)
@@ -0,0 +1,26 @@
+function foo() {
+ var a = arguments;
+ return new Array(a[0]);
+}
+
+function bar(x) {
+ return foo(x);
+}
+
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i) {
+ var result = bar(42);
+ if (result.length != 42)
+ throw "Error: bad result length: " + result;
+}
+
+var didThrow = false;
+try {
+ bar(-1);
+} catch (e) {
+ didThrow = e;
+}
+
+if (("" + didThrow).indexOf("RangeError") != 0)
+ throw "Error: did not throw right exception: " + didThrow;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
