DFG SSA should use GetLocal for arguments, and the GetArgument node type should be removed
https://bugs.webkit.org/show_bug.cgi?id=141623
Reviewed by Oliver Hunt.
During development of https://bugs.webkit.org/show_bug.cgi?id=141332, I realized that I
needed to use GetArgument for loading something that has magically already appeared on the
stack, so currently trunk sort of allows this. But then I realized three things:
- A GetArgument with a non-JSValue flush format means speculating that the value on the
stack obeys that format, rather than just assuming that that it already has that format.
In bug 141332, I want it to assume rather than speculate. That also happens to be more
intuitive; I don't think I was wrong to expect that.
- The node I really want is GetLocal. I'm just getting the value of the local and I don't
want to do anything else.
- Maybe it would be easier if we just used GetLocal for all of the cases where we currently
use GetArgument.
This changes the FTL to do argument speculations in the prologue just like the DFG does.
This brings some consistency to our system, and allows us to get rid of the GetArgument
node. The speculations that the FTL must do are now made explicit in the m_argumentFormats
vector in DFG::Graph. This has natural DCE behavior: even if all uses of the argument are
dead we will still speculate. We already have safeguards to ensure we only speculate if
there are uses that benefit from speculation (which is a much more conservative criterion
than DCE).
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGDCEPhase.cpp:
(JSC::DFG::DCEPhase::run):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGFlushFormat.h:
(JSC::DFG::typeFilterFor):
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::dump):
* dfg/DFGGraph.h:
(JSC::DFG::Graph::valueProfileFor):
(JSC::DFG::Graph::methodOfGettingAValueProfileFor):
* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::initialize):
* dfg/DFGNode.cpp:
(JSC::DFG::Node::hasVariableAccessData):
* dfg/DFGNodeType.h:
* dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
(JSC::DFG::OSRAvailabilityAnalysisPhase::run):
(JSC::DFG::LocalOSRAvailabilityCalculator::executeNode):
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGPutLocalSinkingPhase.cpp:
* dfg/DFGSSAConversionPhase.cpp:
(JSC::DFG::SSAConversionPhase::run):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::lower):
(JSC::FTL::LowerDFGToLLVM::compileNode):
(JSC::FTL::LowerDFGToLLVM::compileGetLocal):
(JSC::FTL::LowerDFGToLLVM::compileGetArgument): Deleted.
* tests/stress/dead-speculating-argument-use.js: Added.
(foo):
(o.valueOf):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@180160 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index b4c57e8..a78df2d 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,80 @@
+2015-02-16 Filip Pizlo <fpizlo@apple.com>
+
+ DFG SSA should use GetLocal for arguments, and the GetArgument node type should be removed
+ https://bugs.webkit.org/show_bug.cgi?id=141623
+
+ Reviewed by Oliver Hunt.
+
+ During development of https://bugs.webkit.org/show_bug.cgi?id=141332, I realized that I
+ needed to use GetArgument for loading something that has magically already appeared on the
+ stack, so currently trunk sort of allows this. But then I realized three things:
+
+ - A GetArgument with a non-JSValue flush format means speculating that the value on the
+ stack obeys that format, rather than just assuming that that it already has that format.
+ In bug 141332, I want it to assume rather than speculate. That also happens to be more
+ intuitive; I don't think I was wrong to expect that.
+
+ - The node I really want is GetLocal. I'm just getting the value of the local and I don't
+ want to do anything else.
+
+ - Maybe it would be easier if we just used GetLocal for all of the cases where we currently
+ use GetArgument.
+
+ This changes the FTL to do argument speculations in the prologue just like the DFG does.
+ This brings some consistency to our system, and allows us to get rid of the GetArgument
+ node. The speculations that the FTL must do are now made explicit in the m_argumentFormats
+ vector in DFG::Graph. This has natural DCE behavior: even if all uses of the argument are
+ dead we will still speculate. We already have safeguards to ensure we only speculate if
+ there are uses that benefit from speculation (which is a much more conservative criterion
+ than DCE).
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGDCEPhase.cpp:
+ (JSC::DFG::DCEPhase::run):
+ * dfg/DFGDoesGC.cpp:
+ (JSC::DFG::doesGC):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ * dfg/DFGFlushFormat.h:
+ (JSC::DFG::typeFilterFor):
+ * dfg/DFGGraph.cpp:
+ (JSC::DFG::Graph::dump):
+ * dfg/DFGGraph.h:
+ (JSC::DFG::Graph::valueProfileFor):
+ (JSC::DFG::Graph::methodOfGettingAValueProfileFor):
+ * dfg/DFGInPlaceAbstractState.cpp:
+ (JSC::DFG::InPlaceAbstractState::initialize):
+ * dfg/DFGNode.cpp:
+ (JSC::DFG::Node::hasVariableAccessData):
+ * dfg/DFGNodeType.h:
+ * dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
+ (JSC::DFG::OSRAvailabilityAnalysisPhase::run):
+ (JSC::DFG::LocalOSRAvailabilityCalculator::executeNode):
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ (JSC::DFG::PredictionPropagationPhase::propagate):
+ * dfg/DFGPutLocalSinkingPhase.cpp:
+ * dfg/DFGSSAConversionPhase.cpp:
+ (JSC::DFG::SSAConversionPhase::run):
+ * dfg/DFGSafeToExecute.h:
+ (JSC::DFG::safeToExecute):
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * ftl/FTLCapabilities.cpp:
+ (JSC::FTL::canCompile):
+ * ftl/FTLLowerDFGToLLVM.cpp:
+ (JSC::FTL::LowerDFGToLLVM::lower):
+ (JSC::FTL::LowerDFGToLLVM::compileNode):
+ (JSC::FTL::LowerDFGToLLVM::compileGetLocal):
+ (JSC::FTL::LowerDFGToLLVM::compileGetArgument): Deleted.
+ * tests/stress/dead-speculating-argument-use.js: Added.
+ (foo):
+ (o.valueOf):
+
2015-02-15 Filip Pizlo <fpizlo@apple.com>
Rare case profiling should actually work
diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
index bf7d041..bb73e97 100644
--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
@@ -141,18 +141,6 @@
break;
}
- case GetArgument: {
- ASSERT(m_graph.m_form == SSA);
- VariableAccessData* variable = node->variableAccessData();
- AbstractValue& value = m_state.variables().operand(variable->local().offset());
- ASSERT(value.isHeapTop());
- FiltrationResult result =
- value.filter(typeFilterFor(useKindFor(variable->flushFormat())));
- ASSERT_UNUSED(result, result == FiltrationOK);
- forNode(node) = value;
- break;
- }
-
case ExtractOSREntryLocal: {
if (!(node->unlinkedLocal().isArgument())
&& m_graph.m_lazyVars.get(node->unlinkedLocal().toLocal())) {
@@ -170,6 +158,8 @@
case GetLocal: {
VariableAccessData* variableAccessData = node->variableAccessData();
AbstractValue value = m_state.variables().operand(variableAccessData->local().offset());
+ // The value in the local should already be checked.
+ DFG_ASSERT(m_graph, node, value.isType(typeFilterFor(variableAccessData->flushFormat())));
if (value.value())
m_state.setFoundConstants(true);
forNode(node) = value;
diff --git a/Source/JavaScriptCore/dfg/DFGClobberize.h b/Source/JavaScriptCore/dfg/DFGClobberize.h
index dcdb7db..2c35f5c 100644
--- a/Source/JavaScriptCore/dfg/DFGClobberize.h
+++ b/Source/JavaScriptCore/dfg/DFGClobberize.h
@@ -391,7 +391,6 @@
return;
case GetLocal:
- case GetArgument:
read(AbstractHeap(Variables, node->local()));
def(HeapLocation(VariableLoc, AbstractHeap(Variables, node->local())), node);
return;
diff --git a/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp b/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
index ccf0958..9a4bbda 100644
--- a/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
@@ -53,12 +53,21 @@
if (m_graph.m_form == SSA) {
for (BasicBlock* block : m_graph.blocksInPreOrder())
fixupBlock(block);
+
+ // This is like cleanVariables, but has a much simpler approach to GetLocal.
+ for (unsigned i = m_graph.m_arguments.size(); i--;) {
+ Node* node = m_graph.m_arguments[i];
+ if (!node)
+ continue;
+ if (node->op() != Phantom && node->op() != Check && node->shouldGenerate())
+ continue;
+ m_graph.m_arguments[i] = nullptr;
+ }
} else {
RELEASE_ASSERT(m_graph.m_form == ThreadedCPS);
for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex)
fixupBlock(m_graph.block(blockIndex));
-
cleanVariables(m_graph.m_arguments);
}
diff --git a/Source/JavaScriptCore/dfg/DFGDoesGC.cpp b/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
index 5dcad43..61aa9cb 100644
--- a/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
+++ b/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
@@ -53,7 +53,6 @@
case SetLocal:
case MovHint:
case ZombieHint:
- case GetArgument:
case Phantom:
case HardPhantom:
case Upsilon:
diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
index c82b5fb..0dc3bf2 100644
--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
@@ -1033,7 +1033,6 @@
case GetArrayLength:
case Phi:
case Upsilon:
- case GetArgument:
case GetIndexedPropertyStorage:
case GetTypedArrayByteOffset:
case LastNodeType:
diff --git a/Source/JavaScriptCore/dfg/DFGFlushFormat.h b/Source/JavaScriptCore/dfg/DFGFlushFormat.h
index 52a0236..b8a3426 100644
--- a/Source/JavaScriptCore/dfg/DFGFlushFormat.h
+++ b/Source/JavaScriptCore/dfg/DFGFlushFormat.h
@@ -93,6 +93,11 @@
return UntypedUse;
}
+inline SpeculatedType typeFilterFor(FlushFormat format)
+{
+ return typeFilterFor(useKindFor(format));
+}
+
inline DataFormat dataFormatFor(FlushFormat format)
{
switch (format) {
diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
index c34fa6c..8b70cc9 100644
--- a/Source/JavaScriptCore/dfg/DFGGraph.cpp
+++ b/Source/JavaScriptCore/dfg/DFGGraph.cpp
@@ -425,6 +425,10 @@
out.print("\n");
out.print("DFG for ", CodeBlockWithJITType(m_codeBlock, JITCode::DFGJIT), ":\n");
out.print(" Fixpoint state: ", m_fixpointState, "; Form: ", m_form, "; Unification state: ", m_unificationState, "; Ref count state: ", m_refCountState, "\n");
+ if (m_form == SSA)
+ out.print(" Argument formats: ", listDump(m_argumentFormats), "\n");
+ else
+ out.print(" Arguments: ", listDump(m_arguments), "\n");
out.print("\n");
Node* lastNode = 0;
diff --git a/Source/JavaScriptCore/dfg/DFGGraph.h b/Source/JavaScriptCore/dfg/DFGGraph.h
index 8c473f8..c19829b 100644
--- a/Source/JavaScriptCore/dfg/DFGGraph.h
+++ b/Source/JavaScriptCore/dfg/DFGGraph.h
@@ -475,21 +475,19 @@
ValueProfile* valueProfileFor(Node* node)
{
if (!node)
- return 0;
+ return nullptr;
CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
- if (node->op() == GetArgument)
- return profiledBlock->valueProfileForArgument(node->local().toArgument());
-
if (node->hasLocal(*this)) {
- if (m_form == SSA)
- return 0;
if (!node->local().isArgument())
return 0;
int argument = node->local().toArgument();
- if (node->variableAccessData() != m_arguments[argument]->variableAccessData())
- return 0;
+ Node* argumentNode = m_arguments[argument];
+ if (!argumentNode)
+ return nullptr;
+ if (node->variableAccessData() != argumentNode->variableAccessData())
+ return nullptr;
return profiledBlock->valueProfileForArgument(argument);
}
@@ -504,16 +502,19 @@
if (!node)
return MethodOfGettingAValueProfile();
- CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
+ if (ValueProfile* valueProfile = valueProfileFor(node))
+ return MethodOfGettingAValueProfile(valueProfile);
if (node->op() == GetLocal) {
+ CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
+
return MethodOfGettingAValueProfile::fromLazyOperand(
profiledBlock,
LazyOperandValueProfileKey(
node->origin.semantic.bytecodeIndex, node->local()));
}
- return MethodOfGettingAValueProfile(valueProfileFor(node));
+ return MethodOfGettingAValueProfile();
}
bool usesArguments() const
@@ -811,7 +812,37 @@
Bag<FrozenValue> m_frozenValues;
Bag<StorageAccessData> m_storageAccessData;
+
+ // In CPS, this is all of the SetArgument nodes for the arguments in the machine code block
+ // that survived DCE. All of them except maybe "this" will survive DCE, because of the Flush
+ // nodes.
+ //
+ // In SSA, this is all of the GetLocal nodes for the arguments in the machine code block that
+ // may have some speculation in the prologue and survived DCE. Note that to get the speculation
+ // for an argument in SSA, you must use m_argumentFormats, since we still have to speculate
+ // even if the argument got killed. For example:
+ //
+ // function foo(x) {
+ // var tmp = x + 1;
+ // }
+ //
+ // Assume that x is always int during profiling. The ArithAdd for "x + 1" will be dead and will
+ // have a proven check for the edge to "x". So, we will not insert a Check node and we will
+ // kill the GetLocal for "x". But, we must do the int check in the progolue, because that's the
+ // thing we used to allow DCE of ArithAdd. Otherwise the add could be impure:
+ //
+ // var o = {
+ // valueOf: function() { do side effects; }
+ // };
+ // foo(o);
+ //
+ // If we DCE the ArithAdd and we remove the int check on x, then this won't do the side
+ // effects.
Vector<Node*, 8> m_arguments;
+
+ // In CPS, this is meaningless. In SSA, this is the argument speculation that we've locked in.
+ Vector<FlushFormat> m_argumentFormats;
+
SegmentedVector<VariableAccessData, 16> m_variableAccessData;
SegmentedVector<ArgumentPosition, 8> m_argumentPositions;
SegmentedVector<StructureSet, 16> m_structureSet;
diff --git a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
index aa6c13e..9696c6e 100644
--- a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
+++ b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
@@ -96,28 +96,37 @@
root->cfaStructureClobberStateAtTail = StructuresAreWatched;
for (size_t i = 0; i < root->valuesAtHead.numberOfArguments(); ++i) {
root->valuesAtTail.argument(i).clear();
- if (m_graph.m_form == SSA) {
- root->valuesAtHead.argument(i).makeHeapTop();
- continue;
+
+ FlushFormat format;
+ if (m_graph.m_form == SSA)
+ format = m_graph.m_argumentFormats[i];
+ else {
+ Node* node = m_graph.m_arguments[i];
+ if (!node)
+ format = FlushedJSValue;
+ else {
+ ASSERT(node->op() == SetArgument);
+ format = node->variableAccessData()->flushFormat();
+ }
}
- Node* node = root->variablesAtHead.argument(i);
- ASSERT(node->op() == SetArgument);
- if (!node->variableAccessData()->shouldUnboxIfPossible()) {
- root->valuesAtHead.argument(i).makeHeapTop();
- continue;
- }
-
- SpeculatedType prediction =
- node->variableAccessData()->argumentAwarePrediction();
- if (isInt32Speculation(prediction))
+ switch (format) {
+ case FlushedInt32:
root->valuesAtHead.argument(i).setType(SpecInt32);
- else if (isBooleanSpeculation(prediction))
+ break;
+ case FlushedBoolean:
root->valuesAtHead.argument(i).setType(SpecBoolean);
- else if (isCellSpeculation(prediction))
+ break;
+ case FlushedCell:
root->valuesAtHead.argument(i).setType(SpecCell);
- else
+ break;
+ case FlushedJSValue:
root->valuesAtHead.argument(i).makeHeapTop();
+ break;
+ default:
+ DFG_CRASH(m_graph, nullptr, "Bad flush format for argument");
+ break;
+ }
}
for (size_t i = 0; i < root->valuesAtHead.numberOfLocals(); ++i) {
Node* node = root->variablesAtHead.local(i);
diff --git a/Source/JavaScriptCore/dfg/DFGNode.cpp b/Source/JavaScriptCore/dfg/DFGNode.cpp
index 5877878..952c74c 100644
--- a/Source/JavaScriptCore/dfg/DFGNode.cpp
+++ b/Source/JavaScriptCore/dfg/DFGNode.cpp
@@ -74,7 +74,6 @@
case Phi:
return graph.m_form != SSA;
case GetLocal:
- case GetArgument:
case SetLocal:
case SetArgument:
case Flush:
diff --git a/Source/JavaScriptCore/dfg/DFGNodeType.h b/Source/JavaScriptCore/dfg/DFGNodeType.h
index 8a0ea81..4e8e5e6 100644
--- a/Source/JavaScriptCore/dfg/DFGNodeType.h
+++ b/Source/JavaScriptCore/dfg/DFGNodeType.h
@@ -61,7 +61,6 @@
macro(KillLocal, NodeMustGenerate) \
macro(MovHint, 0) \
macro(ZombieHint, 0) \
- macro(GetArgument, NodeResultJS | NodeMustGenerate) \
macro(Phantom, NodeMustGenerate) \
macro(HardPhantom, NodeMustGenerate) /* Like Phantom, but we never remove any of its children. */ \
macro(Check, NodeMustGenerate) /* Used if we want just a type check but not liveness. Non-checking uses will be removed. */\
diff --git a/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp b/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
index f3c8835..85b6f3f 100644
--- a/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
@@ -58,10 +58,11 @@
BasicBlock* root = m_graph.block(0);
root->ssa->availabilityAtHead.m_locals.fill(Availability::unavailable());
- for (unsigned argument = root->ssa->availabilityAtHead.m_locals.numberOfArguments(); argument--;) {
- root->ssa->availabilityAtHead.m_locals.argument(argument) =
- Availability::unavailable().withFlush(
- FlushedAt(FlushedJSValue, virtualRegisterForArgument(argument)));
+ for (unsigned argument = m_graph.m_argumentFormats.size(); argument--;) {
+ FlushedAt flushedAt = FlushedAt(
+ m_graph.m_argumentFormats[argument],
+ virtualRegisterForArgument(argument));
+ root->ssa->availabilityAtHead.m_locals.argument(argument) = Availability(flushedAt);
}
// This could be made more efficient by processing blocks in reverse postorder.
@@ -138,7 +139,7 @@
break;
}
- case GetArgument: {
+ case GetLocal: {
VariableAccessData* variable = node->variableAccessData();
m_availability.m_locals.operand(variable->local()) =
Availability(node, variable->flushedAt());
diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index e0c5444..5126299 100644
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -552,7 +552,6 @@
break;
case Upsilon:
- case GetArgument:
// These don't get inserted until we go into SSA.
RELEASE_ASSERT_NOT_REACHED();
break;
diff --git a/Source/JavaScriptCore/dfg/DFGPutLocalSinkingPhase.cpp b/Source/JavaScriptCore/dfg/DFGPutLocalSinkingPhase.cpp
index 152ba46..7401552 100644
--- a/Source/JavaScriptCore/dfg/DFGPutLocalSinkingPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPutLocalSinkingPhase.cpp
@@ -371,7 +371,7 @@
ssaCalculator.newDef(
operandToVariable.operand(node->local()), block, node->child1().node());
break;
- case GetArgument:
+ case GetLocal:
ssaCalculator.newDef(
operandToVariable.operand(node->local()), block, node);
break;
@@ -450,13 +450,6 @@
break;
}
- case GetArgument: {
- VariableAccessData* variable = node->variableAccessData();
- VirtualRegister operand = variable->local();
- mapping.operand(operand) = node;
- break;
- }
-
case KillLocal: {
deferred.operand(node->unlinkedLocal()) = VariableDeferral();
break;
@@ -489,6 +482,16 @@
preciseLocalClobberize(
m_graph, node, escapeHandler, escapeHandler,
[&] (VirtualRegister, Node*) { });
+
+ // If we're a GetLocal, then we also create a mapping.
+ // FIXME: We should be able to just eliminate such GetLocals, when we know
+ // what their incoming value will be.
+ // https://bugs.webkit.org/show_bug.cgi?id=141624
+ if (node->op() == GetLocal) {
+ VariableAccessData* variable = node->variableAccessData();
+ VirtualRegister operand = variable->local();
+ mapping.operand(operand) = node;
+ }
break;
} }
}
diff --git a/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp b/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
index 03246f3..dd5835f 100644
--- a/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
@@ -73,7 +73,7 @@
}
// Find all SetLocals and create Defs for them. We handle SetArgument by creating a
- // GetArgument.
+ // GetLocal, and recording the flush format.
for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
BasicBlock* block = m_graph.block(blockIndex);
if (!block)
@@ -97,7 +97,9 @@
ASSERT(node->op() == SetArgument);
childNode = m_insertionSet.insertNode(
nodeIndex, node->variableAccessData()->prediction(),
- GetArgument, node->origin, OpInfo(node->variableAccessData()));
+ GetLocal, node->origin, OpInfo(node->variableAccessData()));
+ m_argumentGetters.add(childNode);
+ m_argumentMapping.add(node, childNode);
}
m_calculator.newDef(
@@ -198,14 +200,14 @@
// - PhantomLocal becomes Phantom, and its child is whatever is specified by
// valueForOperand.
//
- // - SetArgument is removed. Note that GetArgument nodes have already been inserted.
+ // - SetArgument is removed. Note that GetLocal nodes have already been inserted.
Operands<Node*> valueForOperand(OperandsLike, m_graph.block(0)->variablesAtHead);
for (BasicBlock* block : m_graph.blocksInPreOrder()) {
valueForOperand.clear();
// CPS will claim that the root block has all arguments live. But we have already done
// the first step of SSA conversion: argument locals are no longer live at head;
- // instead we have GetArgument nodes for extracting the values of arguments. So, we
+ // instead we have GetLocal nodes for extracting the values of arguments. So, we
// skip the at-head available value calculation for the root block.
if (block != m_graph.block(0)) {
for (size_t i = valueForOperand.size(); i--;) {
@@ -293,9 +295,15 @@
}
case GetLocal: {
+ VariableAccessData* variable = node->variableAccessData();
+ if (m_argumentGetters.contains(node)) {
+ if (verbose)
+ dataLog("Mapping: ", variable->local(), " -> ", node, "\n");
+ valueForOperand.operand(variable->local()) = node;
+ break;
+ }
node->children.reset();
- VariableAccessData* variable = node->variableAccessData();
if (variable->isCaptured())
break;
@@ -337,15 +345,6 @@
break;
}
- case GetArgument: {
- VariableAccessData* variable = node->variableAccessData();
- ASSERT(!variable->isCaptured());
- if (verbose)
- dataLog("Mapping: ", variable->local(), " -> ", node, "\n");
- valueForOperand.operand(variable->local()) = node;
- break;
- }
-
default:
break;
}
@@ -392,7 +391,21 @@
block->ssa = std::make_unique<BasicBlock::SSAData>(block);
}
- m_graph.m_arguments.clear();
+ m_graph.m_argumentFormats.resize(m_graph.m_arguments.size());
+ for (unsigned i = m_graph.m_arguments.size(); i--;) {
+ FlushFormat format = FlushedJSValue;
+
+ Node* node = m_argumentMapping.get(m_graph.m_arguments[i]);
+
+ // m_argumentMapping.get could return null for a captured local. That's fine. We only
+ // track the argument loads of those arguments for which we speculate type. We don't
+ // speculate type for captured arguments.
+ if (node)
+ format = node->variableAccessData()->flushFormat();
+
+ m_graph.m_argumentFormats[i] = format;
+ m_graph.m_arguments[i] = node; // Record the load that loads the arguments for the benefit of exit profiling.
+ }
m_graph.m_form = SSA;
@@ -408,6 +421,8 @@
SSACalculator m_calculator;
InsertionSet m_insertionSet;
HashMap<VariableAccessData*, SSACalculator::Variable*> m_ssaVariableForVariable;
+ HashMap<Node*, Node*> m_argumentMapping;
+ HashSet<Node*> m_argumentGetters;
Vector<VariableAccessData*> m_variableForSSAIndex;
};
diff --git a/Source/JavaScriptCore/dfg/DFGSafeToExecute.h b/Source/JavaScriptCore/dfg/DFGSafeToExecute.h
index c84513a..a8c85f0 100644
--- a/Source/JavaScriptCore/dfg/DFGSafeToExecute.h
+++ b/Source/JavaScriptCore/dfg/DFGSafeToExecute.h
@@ -122,7 +122,6 @@
case KillLocal:
case MovHint:
case ZombieHint:
- case GetArgument:
case Phantom:
case HardPhantom:
case Upsilon:
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
index 6bf6b86..1e720ec 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
@@ -4898,7 +4898,6 @@
case LastNodeType:
case Phi:
case Upsilon:
- case GetArgument:
case ExtractOSREntryLocal:
case CheckTierUpInLoop:
case CheckTierUpAtReturn:
diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
index 6b123a7..33c314c 100644
--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
@@ -4993,7 +4993,6 @@
case LastNodeType:
case Phi:
case Upsilon:
- case GetArgument:
case ExtractOSREntryLocal:
case CheckInBounds:
case ArithIMul:
diff --git a/Source/JavaScriptCore/ftl/FTLCapabilities.cpp b/Source/JavaScriptCore/ftl/FTLCapabilities.cpp
index 248bdf5..c050739 100644
--- a/Source/JavaScriptCore/ftl/FTLCapabilities.cpp
+++ b/Source/JavaScriptCore/ftl/FTLCapabilities.cpp
@@ -51,7 +51,6 @@
case KillLocal:
case MovHint:
case ZombieHint:
- case GetArgument:
case Phantom:
case HardPhantom:
case Flush:
diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
index 62c2074..86b831e 100644
--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
@@ -135,6 +135,8 @@
m_prologue = FTL_NEW_BLOCK(m_out, ("Prologue"));
LBasicBlock stackOverflow = FTL_NEW_BLOCK(m_out, ("Stack overflow"));
m_handleExceptions = FTL_NEW_BLOCK(m_out, ("Handle Exceptions"));
+
+ LBasicBlock checkArguments = FTL_NEW_BLOCK(m_out, ("Check arguments"));
for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex) {
m_highBlock = m_graph.block(blockIndex);
@@ -193,7 +195,7 @@
m_out.storePtr(m_out.constIntPtr(codeBlock()), addressFor(JSStack::CodeBlock));
m_out.branch(
- didOverflowStack(), rarely(stackOverflow), usually(lowBlock(m_graph.block(0))));
+ didOverflowStack(), rarely(stackOverflow), usually(checkArguments));
m_out.appendTo(stackOverflow, m_handleExceptions);
m_out.call(m_out.operation(operationThrowStackOverflowError), m_callFrame, m_out.constIntPtr(codeBlock()));
@@ -203,13 +205,58 @@
m_out.constInt32(MacroAssembler::maxJumpReplacementSize()));
m_out.unreachable();
- m_out.appendTo(m_handleExceptions, lowBlock(m_graph.block(0)));
+ m_out.appendTo(m_handleExceptions, checkArguments);
m_ftlState.handleExceptionStackmapID = m_stackmapIDs++;
m_out.call(
m_out.stackmapIntrinsic(), m_out.constInt64(m_ftlState.handleExceptionStackmapID),
m_out.constInt32(MacroAssembler::maxJumpReplacementSize()));
m_out.unreachable();
-
+
+ m_out.appendTo(checkArguments, lowBlock(m_graph.block(0)));
+ availabilityMap().clear();
+ availabilityMap().m_locals = Operands<Availability>(codeBlock()->numParameters(), 0);
+ for (unsigned i = codeBlock()->numParameters(); i--;) {
+ availabilityMap().m_locals.argument(i) =
+ Availability(FlushedAt(FlushedJSValue, virtualRegisterForArgument(i)));
+ }
+ m_codeOriginForExitTarget = CodeOrigin(0);
+ m_codeOriginForExitProfile = CodeOrigin(0);
+ m_node = nullptr;
+ for (unsigned i = codeBlock()->numParameters(); i--;) {
+ Node* node = m_graph.m_arguments[i];
+ VirtualRegister operand = virtualRegisterForArgument(i);
+
+ LValue jsValue = m_out.load64(addressFor(operand));
+
+ if (node) {
+ DFG_ASSERT(m_graph, node, operand == node->variableAccessData()->machineLocal());
+
+ // This is a hack, but it's an effective one. It allows us to do CSE on the
+ // primordial load of arguments. This assumes that the GetLocal that got put in
+ // place of the original SetArgument doesn't have any effects before it. This
+ // should hold true.
+ m_loadedArgumentValues.add(node, jsValue);
+ }
+
+ switch (m_graph.m_argumentFormats[i]) {
+ case FlushedInt32:
+ speculate(BadType, jsValueValue(jsValue), node, isNotInt32(jsValue));
+ break;
+ case FlushedBoolean:
+ speculate(BadType, jsValueValue(jsValue), node, isNotBoolean(jsValue));
+ break;
+ case FlushedCell:
+ speculate(BadType, jsValueValue(jsValue), node, isNotCell(jsValue));
+ break;
+ case FlushedJSValue:
+ break;
+ default:
+ DFG_CRASH(m_graph, node, "Bad flush format for argument");
+ break;
+ }
+ }
+ m_out.jump(lowBlock(m_graph.block(0)));
+
for (BasicBlock* block : preOrder)
compileBlock(block);
@@ -381,9 +428,6 @@
case BooleanToNumber:
compileBooleanToNumber();
break;
- case GetArgument:
- compileGetArgument();
- break;
case ExtractOSREntryLocal:
compileExtractOSREntryLocal();
break;
@@ -981,36 +1025,6 @@
}
}
- void compileGetArgument()
- {
- VariableAccessData* variable = m_node->variableAccessData();
- VirtualRegister operand = variable->machineLocal();
- DFG_ASSERT(m_graph, m_node, operand.isArgument());
-
- LValue jsValue = m_out.load64(addressFor(operand));
-
- switch (useKindFor(variable->flushFormat())) {
- case Int32Use:
- speculate(BadType, jsValueValue(jsValue), m_node, isNotInt32(jsValue));
- setInt32(unboxInt32(jsValue));
- break;
- case CellUse:
- speculate(BadType, jsValueValue(jsValue), m_node, isNotCell(jsValue));
- setJSValue(jsValue);
- break;
- case BooleanUse:
- speculate(BadType, jsValueValue(jsValue), m_node, isNotBoolean(jsValue));
- setBoolean(unboxBoolean(jsValue));
- break;
- case UntypedUse:
- setJSValue(jsValue);
- break;
- default:
- DFG_CRASH(m_graph, m_node, "Bad use kind");
- break;
- }
- }
-
void compileExtractOSREntryLocal()
{
EncodedJSValue* buffer = static_cast<EncodedJSValue*>(
@@ -1020,13 +1034,16 @@
void compileGetLocal()
{
- // GetLocals arise only for captured variables.
+ // GetLocals arise only for captured variables and arguments. For arguments, we might have
+ // already loaded it.
+ if (LValue value = m_loadedArgumentValues.get(m_node)) {
+ setJSValue(value);
+ return;
+ }
VariableAccessData* variable = m_node->variableAccessData();
AbstractValue& value = m_state.variables().operand(variable->local());
- DFG_ASSERT(m_graph, m_node, variable->isCaptured());
-
if (isInt32Speculation(value.m_type))
setInt32(m_out.load32(payloadFor(variable->machineLocal())));
else
@@ -7014,6 +7031,11 @@
HashMap<Node*, LoweredNodeValue> m_storageValues;
HashMap<Node*, LoweredNodeValue> m_doubleValues;
+ // This is a bit of a hack. It prevents LLVM from having to do CSE on loading of arguments.
+ // It's nice to have these optimizations on our end because we can guarantee them a bit better.
+ // Probably also saves LLVM compile time.
+ HashMap<Node*, LValue> m_loadedArgumentValues;
+
HashMap<Node*, LValue> m_phis;
LocalOSRAvailabilityCalculator m_availabilityCalculator;
diff --git a/Source/JavaScriptCore/tests/stress/dead-speculating-argument-use.js b/Source/JavaScriptCore/tests/stress/dead-speculating-argument-use.js
new file mode 100644
index 0000000..5f3ebfc
--- /dev/null
+++ b/Source/JavaScriptCore/tests/stress/dead-speculating-argument-use.js
@@ -0,0 +1,17 @@
+function foo(x) {
+ var tmp = x + 1;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i)
+ foo(i);
+
+var didCall = false;
+var o = {
+ valueOf: function() { didCall = true; }
+};
+
+foo(o);
+if (!didCall)
+ throw "Error: didn't call valueOf";
