- Revision
- 184288
- Author
- [email protected]
- Date
- 2015-05-13 10:39:02 -0700 (Wed, 13 May 2015)
Log Message
REGRESSION(r184260): arguments elimination has stopped working because of Check(UntypedUse:) from SSAConversionPhase
https://bugs.webkit.org/show_bug.cgi?id=144951
Reviewed by Michael Saboff.
There were two issues here:
- In r184260 we expected a small number of possible use kinds in Check nodes, and
UntypedUse was not one of them. That seemed like a sensible assumption because we don't
create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a
Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to
follow the same idiom as everyone else and not create tautological checks.
- It's clearly not very robust to assume that Checks will not be used tautologically. So,
this changes how we validate Checks in the escape analyses. We now use willHaveCheck,
which catches cases that AI would have already marked as unnecessary. It then also uses
a new helper called alreadyChecked(), which allows us to just ask if the check is
unnecessary for objects. That's a good fall-back in case AI hadn't run yet.
* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGMayExit.cpp:
* dfg/DFGObjectAllocationSinkingPhase.cpp:
(JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
* dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run):
* dfg/DFGUseKind.h:
(JSC::DFG::alreadyChecked):
* dfg/DFGVarargsForwardingPhase.cpp:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (184287 => 184288)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-13 17:39:02 UTC (rev 184288)
@@ -1,3 +1,35 @@
+2015-05-13 Filip Pizlo <[email protected]>
+
+ REGRESSION(r184260): arguments elimination has stopped working because of Check(UntypedUse:) from SSAConversionPhase
+ https://bugs.webkit.org/show_bug.cgi?id=144951
+
+ Reviewed by Michael Saboff.
+
+ There were two issues here:
+
+ - In r184260 we expected a small number of possible use kinds in Check nodes, and
+ UntypedUse was not one of them. That seemed like a sensible assumption because we don't
+ create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a
+ Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to
+ follow the same idiom as everyone else and not create tautological checks.
+
+ - It's clearly not very robust to assume that Checks will not be used tautologically. So,
+ this changes how we validate Checks in the escape analyses. We now use willHaveCheck,
+ which catches cases that AI would have already marked as unnecessary. It then also uses
+ a new helper called alreadyChecked(), which allows us to just ask if the check is
+ unnecessary for objects. That's a good fall-back in case AI hadn't run yet.
+
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+ * dfg/DFGMayExit.cpp:
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ (JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
+ * dfg/DFGSSAConversionPhase.cpp:
+ (JSC::DFG::SSAConversionPhase::run):
+ * dfg/DFGUseKind.h:
+ (JSC::DFG::alreadyChecked):
+ * dfg/DFGVarargsForwardingPhase.cpp:
+
+k
2015-05-13 Yusuke Suzuki <[email protected]>
[ES6] Implement String.raw
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-13 17:39:02 UTC (rev 184288)
@@ -63,6 +63,11 @@
// version over LoadStore.
DFG_ASSERT(m_graph, nullptr, m_graph.m_form == SSA);
+ if (verbose) {
+ dataLog("Graph before arguments elimination:\n");
+ m_graph.dump();
+ }
+
identifyCandidates();
if (m_candidates.isEmpty())
return false;
@@ -169,15 +174,13 @@
m_graph.doToChildren(
node,
[&] (Edge edge) {
- switch (edge.useKind()) {
- case CellUse:
- case ObjectUse:
- break;
-
- default:
- escape(edge);
- break;
- }
+ if (edge.willNotHaveCheck())
+ return;
+
+ if (alreadyChecked(edge.useKind(), SpecObject))
+ return;
+
+ escape(edge);
});
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGMayExit.cpp (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGMayExit.cpp 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGMayExit.cpp 2015-05-13 17:39:02 UTC (rev 184288)
@@ -51,12 +51,21 @@
}
switch (edge.useKind()) {
+ // These are shady because nodes that have these use kinds will typically exit for
+ // unrelated reasons. For example CompareEq doesn't usually exit, but if it uses ObjectUse
+ // then it will.
case ObjectUse:
case ObjectOrOtherUse:
+ m_result = true;
+ break;
+
+ // These are shady because they check the structure even if the type of the child node
+ // passes the StringObject type filter.
case StringObjectUse:
case StringOrStringObjectUse:
m_result = true;
break;
+
default:
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2015-05-13 17:39:02 UTC (rev 184288)
@@ -841,28 +841,13 @@
m_graph.doToChildren(
node,
[&] (Edge edge) {
- bool ok = true;
-
- switch (edge.useKind()) {
- case KnownCellUse:
- case CellUse:
- case ObjectUse:
- // All of our allocations will pass this.
- break;
-
- case FunctionUse:
- // Function allocations will pass this.
- if (edge->op() != NewFunction)
- ok = false;
- break;
-
- default:
- ok = false;
- break;
- }
+ if (edge.willNotHaveCheck())
+ return;
- if (!ok)
- escape(edge.node());
+ if (alreadyChecked(edge.useKind(), SpecObject))
+ return;
+
+ escape(edge.node());
});
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGSSAConversionPhase.cpp (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGSSAConversionPhase.cpp 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGSSAConversionPhase.cpp 2015-05-13 17:39:02 UTC (rev 184288)
@@ -276,17 +276,18 @@
case SetLocal: {
VariableAccessData* variable = node->variableAccessData();
+ Node* child = node->child1().node();
if (!!(node->flags() & NodeIsFlushed)) {
node->convertToPutStack(
m_graph.m_stackAccessData.add(
variable->local(), variable->flushFormat()));
} else
- node->setOpAndDefaultFlags(Check);
+ node->remove();
if (verbose)
- dataLog("Mapping: ", variable->local(), " -> ", node->child1().node(), "\n");
- valueForOperand.operand(variable->local()) = node->child1().node();
+ dataLog("Mapping: ", variable->local(), " -> ", child, "\n");
+ valueForOperand.operand(variable->local()) = child;
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGUseKind.h (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGUseKind.h 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGUseKind.h 2015-05-13 17:39:02 UTC (rev 184288)
@@ -213,6 +213,17 @@
}
}
+// Returns true if we've already guaranteed the type
+inline bool alreadyChecked(UseKind kind, SpeculatedType type)
+{
+ // If the check involves the structure then we need to know more than just the type to be sure
+ // that the check is done.
+ if (usesStructure(kind))
+ return false;
+
+ return !(type & ~typeFilterFor(kind));
+}
+
inline UseKind useKindForResult(NodeFlags result)
{
ASSERT(!(result & ~NodeResultMask));
Modified: trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp (184287 => 184288)
--- trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp 2015-05-13 16:48:33 UTC (rev 184287)
+++ trunk/Source/_javascript_Core/dfg/DFGVarargsForwardingPhase.cpp 2015-05-13 17:39:02 UTC (rev 184288)
@@ -109,16 +109,16 @@
m_graph.doToChildren(
node,
[&] (Edge edge) {
- switch (edge.useKind()) {
- case CellUse:
- case ObjectUse:
- if (edge == candidate)
- lastUserIndex = nodeIndex;
- break;
- default:
- sawEscape = true;
- break;
- }
+ if (edge == candidate)
+ lastUserIndex = nodeIndex;
+
+ if (edge.willNotHaveCheck())
+ return;
+
+ if (alreadyChecked(edge.useKind(), SpecObject))
+ return;
+
+ sawEscape = true;
});
if (sawEscape) {
if (verbose)
