DFG SSA should use GetLocal for arguments, and the GetArgument node type should be...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Feb 2015 19:27:37 +0000 (19:27 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Feb 2015 19:27:37 +0000 (19:27 +0000)
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

22 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
Source/JavaScriptCore/dfg/DFGDoesGC.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGFlushFormat.h
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGGraph.h
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
Source/JavaScriptCore/dfg/DFGNode.cpp
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGPutLocalSinkingPhase.cpp
Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/tests/stress/dead-speculating-argument-use.js [new file with mode: 0644]

index b4c57e8..a78df2d 100644 (file)
@@ -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
index bf7d041..bb73e97 100644 (file)
@@ -141,18 +141,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         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 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     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;
index dcdb7db..2c35f5c 100644 (file)
@@ -391,7 +391,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         return;
         
     case GetLocal:
-    case GetArgument:
         read(AbstractHeap(Variables, node->local()));
         def(HeapLocation(VariableLoc, AbstractHeap(Variables, node->local())), node);
         return;
index ccf0958..9a4bbda 100644 (file)
@@ -53,12 +53,21 @@ public:
         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);
         }
         
index 5dcad43..61aa9cb 100644 (file)
@@ -53,7 +53,6 @@ bool doesGC(Graph& graph, Node* node)
     case SetLocal:
     case MovHint:
     case ZombieHint:
-    case GetArgument:
     case Phantom:
     case HardPhantom:
     case Upsilon:
index c82b5fb..0dc3bf2 100644 (file)
@@ -1033,7 +1033,6 @@ private:
         case GetArrayLength:
         case Phi:
         case Upsilon:
-        case GetArgument:
         case GetIndexedPropertyStorage:
         case GetTypedArrayByteOffset:
         case LastNodeType:
index 52a0236..b8a3426 100644 (file)
@@ -93,6 +93,11 @@ inline UseKind useKindFor(FlushFormat format)
     return UntypedUse;
 }
 
+inline SpeculatedType typeFilterFor(FlushFormat format)
+{
+    return typeFilterFor(useKindFor(format));
+}
+
 inline DataFormat dataFormatFor(FlushFormat format)
 {
     switch (format) {
index c34fa6c..8b70cc9 100644 (file)
@@ -425,6 +425,10 @@ void Graph::dump(PrintStream& out, DumpContext* context)
     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;
index 8c473f8..c19829b 100644 (file)
@@ -475,21 +475,19 @@ public:
     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 @@ public:
         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 @@ public:
     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;
index aa6c13e..9696c6e 100644 (file)
@@ -96,28 +96,37 @@ void InPlaceAbstractState::initialize()
     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;
-        }
-        
-        Node* node = root->variablesAtHead.argument(i);
-        ASSERT(node->op() == SetArgument);
-        if (!node->variableAccessData()->shouldUnboxIfPossible()) {
-            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();
+            }
         }
         
-        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);
index 5877878..952c74c 100644 (file)
@@ -74,7 +74,6 @@ bool Node::hasVariableAccessData(Graph& graph)
     case Phi:
         return graph.m_form != SSA;
     case GetLocal:
-    case GetArgument:
     case SetLocal:
     case SetArgument:
     case Flush:
index 8a0ea81..4e8e5e6 100644 (file)
@@ -61,7 +61,6 @@ namespace JSC { namespace DFG {
     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. */\
index f3c8835..85b6f3f 100644 (file)
@@ -58,10 +58,11 @@ public:
         
         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 @@ void LocalOSRAvailabilityCalculator::executeNode(Node* node)
         break;
     }
 
-    case GetArgument: {
+    case GetLocal: {
         VariableAccessData* variable = node->variableAccessData();
         m_availability.m_locals.operand(variable->local()) =
             Availability(node, variable->flushedAt());
index e0c5444..5126299 100644 (file)
@@ -552,7 +552,6 @@ private:
             break;
             
         case Upsilon:
-        case GetArgument:
             // These don't get inserted until we go into SSA.
             RELEASE_ASSERT_NOT_REACHED();
             break;
index 152ba46..7401552 100644 (file)
@@ -371,7 +371,7 @@ public:
                     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 @@ public:
                     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 @@ public:
                     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;
                 } }
             }
index 03246f3..dd5835f 100644 (file)
@@ -73,7 +73,7 @@ public:
         }
         
         // 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 @@ public:
                     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 @@ public:
         //   - 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 @@ public:
                 }
                     
                 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 @@ public:
                     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 @@ public:
             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 @@ private:
     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;
 };
 
index c84513a..a8c85f0 100644 (file)
@@ -122,7 +122,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node)
     case KillLocal:
     case MovHint:
     case ZombieHint:
-    case GetArgument:
     case Phantom:
     case HardPhantom:
     case Upsilon:
index 6bf6b86..1e720ec 100644 (file)
@@ -4898,7 +4898,6 @@ void SpeculativeJIT::compile(Node* node)
     case LastNodeType:
     case Phi:
     case Upsilon:
-    case GetArgument:
     case ExtractOSREntryLocal:
     case CheckTierUpInLoop:
     case CheckTierUpAtReturn:
index 6b123a7..33c314c 100644 (file)
@@ -4993,7 +4993,6 @@ void SpeculativeJIT::compile(Node* node)
     case LastNodeType:
     case Phi:
     case Upsilon:
-    case GetArgument:
     case ExtractOSREntryLocal:
     case CheckInBounds:
     case ArithIMul:
index 248bdf5..c050739 100644 (file)
@@ -51,7 +51,6 @@ inline CapabilityLevel canCompile(Node* node)
     case KillLocal:
     case MovHint:
     case ZombieHint:
-    case GetArgument:
     case Phantom:
     case HardPhantom:
     case Flush:
index 62c2074..86b831e 100644 (file)
@@ -135,6 +135,8 @@ public:
         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 @@ public:
         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 @@ public:
             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 @@ private:
         case BooleanToNumber:
             compileBooleanToNumber();
             break;
-        case GetArgument:
-            compileGetArgument();
-            break;
         case ExtractOSREntryLocal:
             compileExtractOSREntryLocal();
             break;
@@ -981,36 +1025,6 @@ private:
         }
     }
 
-    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 @@ private:
     
     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 @@ private:
     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 (file)
index 0000000..5f3ebfc
--- /dev/null
@@ -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";