Modify how we do SetArgument when we inline varargs calls
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Apr 2019 02:41:38 +0000 (02:41 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Apr 2019 02:41:38 +0000 (02:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196712
<rdar://problem/49605012>

Reviewed by Michael Saboff.

JSTests:

* stress/get-stack-wrong-type-when-inline-varargs.js: Added.
(foo):

Source/JavaScriptCore:

When we inline varargs calls, we guarantee that the number of arguments that
go on the stack are somewhere between the "mandatoryMinimum" and the "limit - 1".
However, we can't statically guarantee that the arguments between these two
ranges was filled out by Load/ForwardVarargs. This is because in the general
case we don't know the argument count statically.

However, we used to always emit SetArgumentDefinitely up to "limit - 1" for
all arguments, even when some arguments aren't guaranteed to be in a valid
state. Emitting these SetArgumentDefinitely were helpful because they let us
handle variable liveness and OSR exit metadata. However, when we converted
to SSA, we ended up emitting a GetStack for each such SetArgumentDefinitely.

This is wrong, as we can't guarantee such SetArgumentDefinitely nodes are
actually looking at a range of the stack that are guaranteed to be initialized.
This patch introduces a new form of SetArgument node: SetArgumentMaybe. In terms
of OSR exit metadata and variable liveness tracking, it behaves like SetArgumentDefinitely.

However, it differs in a couple key ways:
1. In ThreadedCPS, GetLocal(@SetArgumentMaybe) is invalid IR, as this implies
you might be loading uninitialized stack. (This same rule applies when you do
the full data flow reachability analysis over CPS Phis.) If someone logically
wanted to emit code like this, the correct node to emit would be GetArgument,
not GetLocal. For similar reasons, PhantomLocal(@SetArgumentMaybe) is also
invalid IR.
2. To track liveness, Flush(@SetArgumentMaybe) is valid, and is the main user
of SetArgumentMaybe.
3. In SSA conversion, we don't lower SetArgumentMaybe to GetStack, as there
should be no data flow user of SetArgumentMaybe.

SetArgumentDefinitely guarantees that the stack slot is initialized.
SetArgumentMaybe makes no such guarantee.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleVarargsInlining):
* dfg/DFGCPSRethreadingPhase.cpp:
(JSC::DFG::CPSRethreadingPhase::freeUnnecessaryNodes):
(JSC::DFG::CPSRethreadingPhase::canonicalizeGetLocalFor):
(JSC::DFG::CPSRethreadingPhase::canonicalizeFlushOrPhantomLocalFor):
(JSC::DFG::CPSRethreadingPhase::canonicalizeLocalsInBlock):
(JSC::DFG::CPSRethreadingPhase::propagatePhis):
(JSC::DFG::CPSRethreadingPhase::computeIsFlushed):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGCommon.h:
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::endBasicBlock):
* dfg/DFGLiveCatchVariablePreservationPhase.cpp:
(JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch):
* dfg/DFGMaximalFlushInsertionPhase.cpp:
(JSC::DFG::MaximalFlushInsertionPhase::treatRegularBlock):
(JSC::DFG::MaximalFlushInsertionPhase::treatRootBlock):
* dfg/DFGMayExit.cpp:
* dfg/DFGNode.cpp:
(JSC::DFG::Node::hasVariableAccessData):
* dfg/DFGNodeType.h:
* dfg/DFGPhantomInsertionPhase.cpp:
* dfg/DFGPredictionPropagationPhase.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):
* dfg/DFGValidate.cpp:
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244324 268f45cc-cd09-0410-ab3c-d52691b4dbfc

24 files changed:
JSTests/ChangeLog
JSTests/stress/get-stack-wrong-type-when-inline-varargs.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGCommon.h
Source/JavaScriptCore/dfg/DFGDoesGC.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
Source/JavaScriptCore/dfg/DFGMaximalFlushInsertionPhase.cpp
Source/JavaScriptCore/dfg/DFGMayExit.cpp
Source/JavaScriptCore/dfg/DFGNode.cpp
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGPhantomInsertionPhase.cpp
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/dfg/DFGValidate.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp

index e000b2f..325ff0a 100644 (file)
@@ -1,5 +1,16 @@
 2019-04-15  Saam barati  <sbarati@apple.com>
 
+        Modify how we do SetArgument when we inline varargs calls
+        https://bugs.webkit.org/show_bug.cgi?id=196712
+        <rdar://problem/49605012>
+
+        Reviewed by Michael Saboff.
+
+        * stress/get-stack-wrong-type-when-inline-varargs.js: Added.
+        (foo):
+
+2019-04-15  Saam barati  <sbarati@apple.com>
+
         SafeToExecute for GetByOffset/GetGetterByOffset/PutByOffset is using the wrong child for the base
         https://bugs.webkit.org/show_bug.cgi?id=196945
         <rdar://problem/49802750>
diff --git a/JSTests/stress/get-stack-wrong-type-when-inline-varargs.js b/JSTests/stress/get-stack-wrong-type-when-inline-varargs.js
new file mode 100644 (file)
index 0000000..bb70ed9
--- /dev/null
@@ -0,0 +1,10 @@
+//@ runDefault("--useConcurrentJIT=0", "--validateAbstractInterpreterState=1", "--validateAbstractInterpreterStateProbability=1.0", "--forceEagerCompilation=1")
+
+function foo(a, v) {
+    a[0] = v + 2000000000;
+}
+noInline(foo);
+
+for (var i = 0; i < 100000; ++i) {
+    foo({}, 1000000000);
+}
index 501ef26..1a9e7d9 100644 (file)
@@ -1,3 +1,86 @@
+2019-04-15  Saam barati  <sbarati@apple.com>
+
+        Modify how we do SetArgument when we inline varargs calls
+        https://bugs.webkit.org/show_bug.cgi?id=196712
+        <rdar://problem/49605012>
+
+        Reviewed by Michael Saboff.
+
+        When we inline varargs calls, we guarantee that the number of arguments that
+        go on the stack are somewhere between the "mandatoryMinimum" and the "limit - 1".
+        However, we can't statically guarantee that the arguments between these two
+        ranges was filled out by Load/ForwardVarargs. This is because in the general
+        case we don't know the argument count statically.
+        
+        However, we used to always emit SetArgumentDefinitely up to "limit - 1" for
+        all arguments, even when some arguments aren't guaranteed to be in a valid
+        state. Emitting these SetArgumentDefinitely were helpful because they let us
+        handle variable liveness and OSR exit metadata. However, when we converted
+        to SSA, we ended up emitting a GetStack for each such SetArgumentDefinitely.
+        
+        This is wrong, as we can't guarantee such SetArgumentDefinitely nodes are
+        actually looking at a range of the stack that are guaranteed to be initialized.
+        This patch introduces a new form of SetArgument node: SetArgumentMaybe. In terms
+        of OSR exit metadata and variable liveness tracking, it behaves like SetArgumentDefinitely.
+        
+        However, it differs in a couple key ways:
+        1. In ThreadedCPS, GetLocal(@SetArgumentMaybe) is invalid IR, as this implies
+        you might be loading uninitialized stack. (This same rule applies when you do
+        the full data flow reachability analysis over CPS Phis.) If someone logically
+        wanted to emit code like this, the correct node to emit would be GetArgument,
+        not GetLocal. For similar reasons, PhantomLocal(@SetArgumentMaybe) is also
+        invalid IR.
+        2. To track liveness, Flush(@SetArgumentMaybe) is valid, and is the main user
+        of SetArgumentMaybe.
+        3. In SSA conversion, we don't lower SetArgumentMaybe to GetStack, as there
+        should be no data flow user of SetArgumentMaybe.
+        
+        SetArgumentDefinitely guarantees that the stack slot is initialized.
+        SetArgumentMaybe makes no such guarantee.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleVarargsInlining):
+        * dfg/DFGCPSRethreadingPhase.cpp:
+        (JSC::DFG::CPSRethreadingPhase::freeUnnecessaryNodes):
+        (JSC::DFG::CPSRethreadingPhase::canonicalizeGetLocalFor):
+        (JSC::DFG::CPSRethreadingPhase::canonicalizeFlushOrPhantomLocalFor):
+        (JSC::DFG::CPSRethreadingPhase::canonicalizeLocalsInBlock):
+        (JSC::DFG::CPSRethreadingPhase::propagatePhis):
+        (JSC::DFG::CPSRethreadingPhase::computeIsFlushed):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGCommon.h:
+        * dfg/DFGDoesGC.cpp:
+        (JSC::DFG::doesGC):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGInPlaceAbstractState.cpp:
+        (JSC::DFG::InPlaceAbstractState::endBasicBlock):
+        * dfg/DFGLiveCatchVariablePreservationPhase.cpp:
+        (JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch):
+        * dfg/DFGMaximalFlushInsertionPhase.cpp:
+        (JSC::DFG::MaximalFlushInsertionPhase::treatRegularBlock):
+        (JSC::DFG::MaximalFlushInsertionPhase::treatRootBlock):
+        * dfg/DFGMayExit.cpp:
+        * dfg/DFGNode.cpp:
+        (JSC::DFG::Node::hasVariableAccessData):
+        * dfg/DFGNodeType.h:
+        * dfg/DFGPhantomInsertionPhase.cpp:
+        * dfg/DFGPredictionPropagationPhase.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):
+        * dfg/DFGValidate.cpp:
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+
 2019-04-15  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r243672.
index 0fc9be6..74d82ea 100644 (file)
@@ -323,7 +323,8 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     }
         
     case SetArgumentDefinitely:
-        // Assert that the state of arguments has been set. SetArgumentDefinitely means
+    case SetArgumentMaybe:
+        // Assert that the state of arguments has been set. SetArgumentDefinitely/SetArgumentMaybe means
         // that someone set the argument values out-of-band, and currently this always means setting to a
         // non-clear value.
         ASSERT(!m_state.operand(node->local()).isClear());
index a71d298..be99551 100644 (file)
@@ -1863,6 +1863,7 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
         m_currentBlock->variablesAtTail.setOperand(countVariable->local(), setArgumentCount);
         
         set(VirtualRegister(argumentStart), get(thisArgument), ImmediateNakedSet);
+        unsigned numSetArguments = 0;
         for (unsigned argument = 1; argument < maxNumArguments; ++argument) {
             VariableAccessData* variable = newVariableAccessData(VirtualRegister(remappedArgumentStart + argument));
             variable->mergeShouldNeverUnbox(true); // We currently have nowhere to put the type check on the LoadVarargs. LoadVarargs is effectful, so after it finishes, we cannot exit.
@@ -1882,8 +1883,9 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
                 variable->predict(profile.computeUpdatedPrediction(locker));
             }
             
-            Node* setArgument = addToGraph(SetArgumentDefinitely, OpInfo(variable));
+            Node* setArgument = addToGraph(numSetArguments >= mandatoryMinimum ? SetArgumentMaybe : SetArgumentDefinitely, OpInfo(variable));
             m_currentBlock->variablesAtTail.setOperand(variable->local(), setArgument);
+            ++numSetArguments;
         }
     };
 
index 7989249..d98fd4a 100644 (file)
@@ -94,6 +94,9 @@ private:
                         continue;
                     }
                     switch (node->child1()->op()) {
+                    case SetArgumentMaybe:
+                        DFG_CRASH(m_graph, node, "Invalid Phantom(@SetArgumentMaybe)");
+                        break;
                     case Phi:
                     case SetArgumentDefinitely:
                     case SetLocal:
@@ -169,12 +172,14 @@ private:
                     m_block->variablesAtTail.atFor<operandKind>(idx) = node;
                     return;
                 }
+                ASSERT(otherNode->op() != SetArgumentMaybe);
                 ASSERT(otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely);
                 break;
             default:
                 break;
             }
             
+            ASSERT(otherNode->op() != SetArgumentMaybe);
             ASSERT(otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely || otherNode->op() == GetLocal);
             ASSERT(otherNode->variableAccessData() == variable);
             
@@ -230,7 +235,7 @@ private:
                 break;
             }
             
-            ASSERT(otherNode->op() == Phi || otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely);
+            ASSERT(otherNode->op() == Phi || otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely || otherNode->op() == SetArgumentMaybe);
             
             if (nodeType == PhantomLocal && otherNode->op() == SetLocal) {
                 // PhantomLocal(SetLocal) doesn't make sense. PhantomLocal means: at this
@@ -297,12 +302,12 @@ private:
             // The rules for threaded CPS form:
             // 
             // Head variable: describes what is live at the head of the basic block.
-            // Head variable links may refer to Flush, PhantomLocal, Phi, or SetArgumentDefinitely.
-            // SetArgumentDefinitely may only appear in the root block.
+            // Head variable links may refer to Flush, PhantomLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe.
+            // SetArgumentDefinitely/SetArgumentMaybe may only appear in the root block.
             //
             // Tail variable: the last thing that happened to the variable in the block.
-            // It may be a Flush, PhantomLocal, GetLocal, SetLocal, SetArgumentDefinitely, or Phi.
-            // SetArgumentDefinitely may only appear in the root block. Note that if there ever
+            // It may be a Flush, PhantomLocal, GetLocal, SetLocal, SetArgumentDefinitely/SetArgumentMaybe, or Phi.
+            // SetArgumentDefinitely/SetArgumentMaybe may only appear in the root block. Note that if there ever
             // was a GetLocal to the variable, and it was followed by PhantomLocals and
             // Flushes but not SetLocals, then the tail variable will be the GetLocal.
             // This reflects the fact that you only care that the tail variable is a
@@ -311,22 +316,23 @@ private:
             // variable will be a SetLocal and not those subsequent Flushes.
             //
             // Child of GetLocal: the operation that the GetLocal keeps alive. It may be
-            // a Phi from the current block. For arguments, it may be a SetArgumentDefinitely.
+            // a Phi from the current block. For arguments, it may be a SetArgumentDefinitely
+            // but it can't be a SetArgumentMaybe.
             //
             // Child of SetLocal: must be a value producing node.
             //
             // Child of Flush: it may be a Phi from the current block or a SetLocal. For
-            // arguments it may also be a SetArgumentDefinitely.
+            // arguments it may also be a SetArgumentDefinitely/SetArgumentMaybe.
             //
             // Child of PhantomLocal: it may be a Phi from the current block. For
-            // arguments it may also be a SetArgumentDefinitely.
+            // arguments it may also be a SetArgumentDefinitely/SetArgumentMaybe.
             //
             // Children of Phi: other Phis in the same basic block, or any of the
-            // following from predecessor blocks: SetLocal, Phi, or SetArgumentDefinitely.
+            // following from predecessor blocks: SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe.
             // These are computed by looking at the tail variables of the predecessor blocks
-            // and either using it directly (if it's a SetLocal, Phi, or SetArgumentDefinitely) or
+            // and either using it directly (if it's a SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe) or
             // loading that nodes child (if it's a GetLocal, PhanomLocal, or Flush - all
-            // of these will have children that are SetLocal, Phi, or SetArgumentDefinitely).
+            // of these will have children that are SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe).
             
             switch (node->op()) {
             case GetLocal:
@@ -346,6 +352,7 @@ private:
                 break;
                 
             case SetArgumentDefinitely:
+            case SetArgumentMaybe:
                 canonicalizeSet(node);
                 break;
                 
@@ -420,7 +427,8 @@ private:
                 ASSERT(
                     variableInPrevious->op() == SetLocal
                     || variableInPrevious->op() == Phi
-                    || variableInPrevious->op() == SetArgumentDefinitely);
+                    || variableInPrevious->op() == SetArgumentDefinitely
+                    || variableInPrevious->op() == SetArgumentMaybe);
           
                 if (!currentPhi->child1()) {
                     currentPhi->children.setChild1(Edge(variableInPrevious));
@@ -483,6 +491,7 @@ private:
             switch (node->op()) {
             case SetLocal:
             case SetArgumentDefinitely:
+            case SetArgumentMaybe:
                 break;
                 
             case Flush:
index dec1588..b1598d4 100644 (file)
@@ -451,6 +451,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
     case Phi:
     case PhantomLocal:
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case Jump:
     case Branch:
     case Switch:
index e231cfb..5996b76 100644 (file)
@@ -173,7 +173,7 @@ enum GraphForm {
     // most likely) then it implies that the local is still live but that it need
     // not be stored to the stack necessarily. This implies that Phantom can
     // reference nodes that have no result, as long as those nodes are valid
-    // GetLocal children (i.e. Phi, SetLocal, SetArgumentDefinitely).
+    // GetLocal children (i.e. Phi, SetLocal, SetArgumentDefinitely, SetArgumentMaybe).
     //
     // LoadStore form also implies that Phis need not have children. By default,
     // they end up having no children if you enter LoadStore using the canonical
index 5071d21..10991a5 100644 (file)
@@ -79,6 +79,7 @@ bool doesGC(Graph& graph, Node* node)
     case Flush:
     case PhantomLocal:
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case ArithBitNot:
     case ArithBitAnd:
     case ArithBitOr:
index 25d445d..a0cd01a 100644 (file)
@@ -2325,6 +2325,7 @@ private:
 #if !ASSERT_DISABLED
         // Have these no-op cases here to ensure that nobody forgets to add handlers for new opcodes.
         case SetArgumentDefinitely:
+        case SetArgumentMaybe:
         case JSConstant:
         case LazyJSConstant:
         case DoubleConstant:
index 1afb6bc..d8fba2e 100644 (file)
@@ -235,6 +235,7 @@ bool InPlaceAbstractState::endBasicBlock()
             switch (node->op()) {
             case Phi:
             case SetArgumentDefinitely:
+            case SetArgumentMaybe:
             case PhantomLocal:
             case Flush: {
                 // The block transfers the value from head to tail.
index e2b19d6..d3cbba2 100644 (file)
@@ -198,7 +198,7 @@ public:
                 currentExceptionHandler = newHandler;
             }
 
-            if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely)) {
+            if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) {
                 InlineCallFrame* inlineCallFrame = node->origin.semantic.inlineCallFrame();
                 if (inlineCallFrame)
                     seenInlineCallFrames.add(inlineCallFrame);
index 57dbea3..394a8e6 100644 (file)
@@ -76,7 +76,7 @@ public:
                         isPrimordialSetArgument = node == iter->value[node->local().toArgument()];
                 }
 
-                if (node->op() == SetLocal || (node->op() == SetArgumentDefinitely && !isPrimordialSetArgument)) {
+                if (node->op() == SetLocal || (node->op() == SetArgumentDefinitely && !isPrimordialSetArgument) || node->op() == SetArgumentMaybe) {
                     VirtualRegister operand = node->local();
                     VariableAccessData* flushAccessData = currentBlockAccessData.operand(operand);
                     if (!flushAccessData)
@@ -136,7 +136,7 @@ public:
 
         for (unsigned i = 0; i < block->variablesAtTail.numberOfLocals(); i++) {
             VirtualRegister operand = virtualRegisterForLocal(i);
-            DFG_ASSERT(m_graph, nullptr, initialAccessNodes.operand(operand)->op() == Flush); // We should have inserted a Flush before any SetLocal/SetArgumentDefinitely for the local that we are analyzing now.
+            DFG_ASSERT(m_graph, nullptr, initialAccessNodes.operand(operand)->op() == Flush); // We should have inserted a Flush before any SetLocal/SetArgumentDefinitely/SetArgumentMaybe for the local that we are analyzing now.
             VariableAccessData* accessData = initialAccessData.operand(operand);
             DFG_ASSERT(m_graph, nullptr, accessData);
             insertionSet.insertNode(0, SpecNone, 
index c26203a..df5cf6d 100644 (file)
@@ -48,6 +48,7 @@ ExitMode mayExitImpl(Graph& graph, Node* node, StateType& state)
     // conservative when maintaining this list, because adding new node types to it doesn't
     // generally make things a lot better but it might introduce subtle bugs.
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case JSConstant:
     case DoubleConstant:
     case LazyJSConstant:
index ef14f44..760aa58 100644 (file)
@@ -74,6 +74,7 @@ bool Node::hasVariableAccessData(Graph& graph)
     case GetLocal:
     case SetLocal:
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case Flush:
     case PhantomLocal:
         return true;
index d82b3b5..7f67872 100644 (file)
@@ -103,8 +103,10 @@ namespace JSC { namespace DFG {
     macro(CheckTierUpAndOSREnter, NodeMustGenerate) \
     macro(CheckTierUpAtReturn, NodeMustGenerate) \
     \
-    /* Marker for an argument being set at the prologue of a function. */\
+    /* Marker for an argument being set at the prologue of a function. The argument is guaranteed to be set after this node. */\
     macro(SetArgumentDefinitely, 0) \
+    /* A marker like the above that we use to track variable liveness and OSR exit state. However, it's not guaranteed to be set. To verify it was set, you'd need to check the actual argument length. We use this for varargs when we're unsure how many argument may actually end up on the stack. */\
+    macro(SetArgumentMaybe, 0) \
     \
     /* Marker of location in the IR where we may possibly perform jump replacement to */\
     /* invalidate this code block. */\
index 0ea2a7e..96fa23d 100644 (file)
@@ -117,6 +117,7 @@ private:
 
             case GetLocal:
             case SetArgumentDefinitely:
+            case SetArgumentMaybe:
                 m_values.operand(node->local()) = nullptr;
                 break;
                 
index e0f4e05..6e3fb25 100644 (file)
@@ -1254,6 +1254,7 @@ private:
         case ProfileControlFlow:
         case ForceOSRExit:
         case SetArgumentDefinitely:
+        case SetArgumentMaybe:
         case SetFunctionName:
         case CheckStructure:
         case CheckCell:
index a3117bf..871595b 100644 (file)
@@ -261,6 +261,8 @@ public:
         //     valueForOperand.
         //
         //   - SetArgumentDefinitely is removed. Note that GetStack nodes have already been inserted.
+        //
+        //   - SetArgumentMaybe is removed. It should not have any data flow uses.
         Operands<Node*> valueForOperand(OperandsLike, m_graph.block(0)->variablesAtHead);
         for (BasicBlock* block : m_graph.blocksInPreOrder()) {
             valueForOperand.clear();
@@ -393,6 +395,11 @@ public:
                     break;
                 }
 
+                case SetArgumentMaybe: {
+                    node->remove(m_graph);
+                    break;
+                }
+                    
                 default:
                     break;
                 }
index 472bb68..992bcf8 100644 (file)
@@ -198,6 +198,7 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
     case Flush:
     case PhantomLocal:
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case ArithBitNot:
     case ArithBitAnd:
     case ArithBitOr:
index 614ed4d..1e33745 100644 (file)
@@ -1973,6 +1973,7 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
         // This is a no-op; it just marks the fact that the argument is being used.
         // But it may be profitable to use this as a hook to run speculation checks
         // on arguments, thereby allowing us to trivially eliminate such checks if
index 1d27a8f..f009e6b 100644 (file)
@@ -2072,6 +2072,7 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
         // This is a no-op; it just marks the fact that the argument is being used.
         // But it may be profitable to use this as a hook to run speculation checks
         // on arguments, thereby allowing us to trivially eliminate such checks if
index daa3c83..04dfdcb 100644 (file)
@@ -494,7 +494,7 @@ private:
                         (node, edge),
                         edge->op() == SetLocal
                         || edge->op() == SetArgumentDefinitely
-                        || edge->op() == Flush
+                        || edge->op() == SetArgumentMaybe
                         || edge->op() == Phi);
                     
                     if (phisInThisBlock.contains(edge.node()))
@@ -505,7 +505,7 @@ private:
                             (node, edge),
                             edge->op() == SetLocal
                             || edge->op() == SetArgumentDefinitely
-                            || edge->op() == Flush);
+                            || edge->op() == SetArgumentMaybe);
                         
                         continue;
                     }
@@ -537,6 +537,7 @@ private:
                             (local, block->predecessors[k], prevNode),
                             prevNode->op() == SetLocal
                             || prevNode->op() == SetArgumentDefinitely
+                            || prevNode->op() == SetArgumentMaybe
                             || prevNode->op() == Phi);
                         if (prevNode == edge.node()) {
                             found = true;
@@ -666,6 +667,7 @@ private:
                     if (m_graph.m_form == ThreadedCPS) {
                         VALIDATE((node, block), getLocalPositions.operand(node->local()) == notSet);
                         VALIDATE((node, block), !!node->child1());
+                        VALIDATE((node, block), node->child1()->op() == SetArgumentDefinitely || node->child1()->op() == Phi);
                     }
                     getLocalPositions.operand(node->local()) = i;
                     break;
@@ -682,6 +684,20 @@ private:
                     getLocalPositions.operand(node->local()) = notSet;
                     setLocalPositions.operand(node->local()) = notSet;
                     break;
+                case SetArgumentMaybe:
+                    break;
+                case Flush:
+                case PhantomLocal:
+                    if (m_graph.m_form == ThreadedCPS) {
+                        VALIDATE((node, block), 
+                            node->child1()->op() == Phi
+                            || node->child1()->op() == SetLocal
+                            || node->child1()->op() == SetArgumentDefinitely
+                            || node->child1()->op() == SetArgumentMaybe);
+                        if (node->op() == PhantomLocal)
+                            VALIDATE((node, block), node->child1()->op() != SetArgumentMaybe);
+                    }
+                    break;
                 default:
                     break;
                 }
@@ -699,6 +715,52 @@ private:
                     block, getLocalPositions, setLocalPositions, virtualRegisterForLocal(i));
             }
         }
+
+        if (m_graph.m_form == ThreadedCPS) {
+            Vector<Node*> worklist;
+            HashSet<Node*> seen;
+            for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
+                for (Node* node : *block) {
+                    if (node->op() == GetLocal || node->op() == PhantomLocal) {
+                        worklist.append(node);
+                        auto addResult = seen.add(node);
+                        VALIDATE((node, block), addResult.isNewEntry);
+                    }
+                }
+            }
+
+            while (worklist.size()) {
+                Node* node = worklist.takeLast();
+                switch (node->op()) {
+                case PhantomLocal:
+                case GetLocal: {
+                    Node* child = node->child1().node();
+                    if (seen.add(child).isNewEntry)
+                        worklist.append(child);
+                    break;
+                }
+                case Phi: {
+                    for (unsigned i = 0; i < m_graph.numChildren(node); ++i) {
+                        Edge edge = m_graph.child(node, i);
+                        if (!edge)
+                            continue;
+                        if (seen.add(edge.node()).isNewEntry)
+                            worklist.append(edge.node());
+                    }
+                    break;
+                }
+                case SetLocal:
+                case SetArgumentDefinitely:
+                    break;
+                case SetArgumentMaybe:
+                    VALIDATE((node), !"Should not reach SetArgumentMaybe. GetLocal that has data flow that reaches a SetArgumentMaybe is invalid IR.");
+                    break;
+                default:
+                    VALIDATE((node), !"Unexecpted node type.");
+                    break;
+                }
+            }
+        }
     }
     
     void validateSSA()
@@ -740,6 +802,7 @@ private:
                 case GetLocal:
                 case SetLocal:
                 case SetArgumentDefinitely:
+                case SetArgumentMaybe:
                 case Phantom:
                     VALIDATE((node), !"bad node type for SSA");
                     break;
index e729d35..06e2134 100644 (file)
@@ -57,6 +57,7 @@ inline CapabilityLevel canCompile(Node* node)
     case Flush:
     case PhantomLocal:
     case SetArgumentDefinitely:
+    case SetArgumentMaybe:
     case Return:
     case ArithBitNot:
     case ArithBitAnd: