Add SetCallee as DFG-Operation
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 May 2018 15:42:11 +0000 (15:42 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 May 2018 15:42:11 +0000 (15:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184582

Patch by Dominik Infuehr <dinfuehr@igalia.com> on 2018-05-01
Reviewed by Filip Pizlo.

JSTests:

Added test that runs into infinite loop without updating the callee and
therefore emitting SetCallee in DFG for recursive tail calls.

* stress/closure-recursive-tail-call-infinite-loop.js: Added.
(Foo):
(second):
(first):
(return.closure):
(createClosure):

Source/JavaScriptCore:

For recursive tail calls not only the argument count can change but also the
callee. Add SetCallee to DFG that sets the callee slot in the current call frame.
Also update the callee when optimizing a recursive tail call.
Enable recursive tail call optimization also for closures.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleRecursiveTailCall):
(JSC::DFG::ByteCodeParser::handleCallVariant):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGMayExit.cpp:
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileSetCallee):
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::compileSetCallee):

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

18 files changed:
JSTests/ChangeLog
JSTests/stress/closure-recursive-tail-call-infinite-loop.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGDoesGC.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGMayExit.cpp
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 636a83c..73e00d8 100644 (file)
@@ -1,3 +1,20 @@
+2018-05-01  Dominik Infuehr  <dinfuehr@igalia.com>
+
+        Add SetCallee as DFG-Operation
+        https://bugs.webkit.org/show_bug.cgi?id=184582
+
+        Reviewed by Filip Pizlo.
+
+        Added test that runs into infinite loop without updating the callee and
+        therefore emitting SetCallee in DFG for recursive tail calls.
+
+        * stress/closure-recursive-tail-call-infinite-loop.js: Added.
+        (Foo):
+        (second):
+        (first):
+        (return.closure):
+        (createClosure):
+
 2018-04-30  Saam Barati  <sbarati@apple.com>
 
         ToString constant folds without preserving checks, causing us to break assumptions that the code would OSR exit
diff --git a/JSTests/stress/closure-recursive-tail-call-infinite-loop.js b/JSTests/stress/closure-recursive-tail-call-infinite-loop.js
new file mode 100644 (file)
index 0000000..dfd96a0
--- /dev/null
@@ -0,0 +1,29 @@
+"use strict";
+
+function Foo() {}
+
+function second(next, cp) {
+  return 100;
+}
+
+function first(next, cp) {
+  return cp < 60 ? new Foo() : next(cp);
+}
+
+function createClosure(next, strategy) {
+  return function closure(cp) {
+    return strategy(next, cp);
+  };
+}
+
+var tmp = createClosure(null, second);
+var bar = createClosure(tmp, first);
+
+noInline(bar);
+
+for (var i=0; i<50000; i++) {
+  bar(32);
+  bar(32);
+  bar(32);
+  bar(100);
+}
index 28fd4e3..e4a3635 100644 (file)
@@ -1,3 +1,44 @@
+2018-05-01  Dominik Infuehr  <dinfuehr@igalia.com>
+
+        Add SetCallee as DFG-Operation
+        https://bugs.webkit.org/show_bug.cgi?id=184582
+
+        Reviewed by Filip Pizlo.
+
+        For recursive tail calls not only the argument count can change but also the
+        callee. Add SetCallee to DFG that sets the callee slot in the current call frame.
+        Also update the callee when optimizing a recursive tail call.
+        Enable recursive tail call optimization also for closures.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleRecursiveTailCall):
+        (JSC::DFG::ByteCodeParser::handleCallVariant):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGDoesGC.cpp:
+        (JSC::DFG::doesGC):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGMayExit.cpp:
+        * dfg/DFGNodeType.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::safeToExecute):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileSetCallee):
+        * dfg/DFGSpeculativeJIT.h:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
+        (JSC::FTL::DFG::LowerDFGToB3::compileSetCallee):
+
 2018-05-01  Oleksandr Skachkov  <gskachkov@gmail.com>
 
         WebAssembly: add support for stream APIs - JavaScript API
index c3afd0c..c6c98db 100644 (file)
@@ -2448,6 +2448,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         forNode(node).setType(SpecInt32Only);
         break;
 
+    case SetCallee:
     case SetArgumentCountIncludingThis:
         break;
         
index 66132fa..1cb947a 100644 (file)
@@ -156,7 +156,7 @@ private:
     void emitArgumentPhantoms(int registerOffset, int argumentCountIncludingThis);
     Node* getArgumentCount();
     template<typename ChecksFunctor>
-    bool handleRecursiveTailCall(CallVariant, int registerOffset, int argumentCountIncludingThis, const ChecksFunctor& emitFunctionCheckIfNeeded);
+    bool handleRecursiveTailCall(Node* callTargetNode, CallVariant, int registerOffset, int argumentCountIncludingThis, const ChecksFunctor& emitFunctionCheckIfNeeded);
     unsigned inliningCost(CallVariant, int argumentCountIncludingThis, InlineCallFrame::Kind); // Return UINT_MAX if it's not an inlining candidate. By convention, intrinsics have a cost of 1.
     // Handle inlining. Return true if it succeeded, false if we need to plant a call.
     bool handleVarargsInlining(Node* callTargetNode, int resultOperand, const CallLinkStatus&, int registerOffset, VirtualRegister thisArgument, VirtualRegister argumentsArgument, unsigned argumentsOffset, NodeType callOp, InlineCallFrame::Kind);
@@ -1356,16 +1356,11 @@ void ByteCodeParser::emitArgumentPhantoms(int registerOffset, int argumentCountI
 }
 
 template<typename ChecksFunctor>
-bool ByteCodeParser::handleRecursiveTailCall(CallVariant callVariant, int registerOffset, int argumentCountIncludingThis, const ChecksFunctor& emitFunctionCheckIfNeeded)
+bool ByteCodeParser::handleRecursiveTailCall(Node* callTargetNode, CallVariant callVariant, int registerOffset, int argumentCountIncludingThis, const ChecksFunctor& emitFunctionCheckIfNeeded)
 {
     if (UNLIKELY(!Options::optimizeRecursiveTailCalls()))
         return false;
 
-    // Currently we cannot do this optimisation for closures. The problem is that "emitFunctionChecks" which we use later is too coarse, only checking the executable
-    // and not the value of captured variables.
-    if (callVariant.isClosureCall())
-        return false;
-
     auto targetExecutable = callVariant.executable();
     InlineStackEntry* stackEntry = m_inlineStackTop;
     do {
@@ -1385,11 +1380,25 @@ bool ByteCodeParser::handleRecursiveTailCall(CallVariant callVariant, int regist
                 return false;
         }
 
+        // If an InlineCallFrame is not a closure, it was optimized using a constant callee.
+        // Check if this is the same callee that we try to inline here.
+        if (stackEntry->m_inlineCallFrame && !stackEntry->m_inlineCallFrame->isClosureCall) {
+            if (stackEntry->m_inlineCallFrame->calleeConstant() != callVariant.function())
+                continue;
+        }
+
         // We must add some check that the profiling information was correct and the target of this call is what we thought.
         emitFunctionCheckIfNeeded();
         // We flush everything, as if we were in the backedge of a loop (see treatment of op_jmp in parseBlock).
         flushForTerminal();
 
+        // We must set the callee to the right value
+        if (stackEntry->m_inlineCallFrame) {
+            if (stackEntry->m_inlineCallFrame->isClosureCall)
+                setDirect(stackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)), callTargetNode, NormalSet);
+        } else
+            addToGraph(SetCallee, callTargetNode);
+
         // We must set the arguments to the right values
         if (!stackEntry->m_inlineCallFrame)
             addToGraph(SetArgumentCountIncludingThis, OpInfo(argumentCountIncludingThis));
@@ -1683,7 +1692,7 @@ ByteCodeParser::CallOptimizationResult ByteCodeParser::handleCallVariant(Node* c
         didInsertChecks = true;
     };
 
-    if (kind == InlineCallFrame::TailCall && ByteCodeParser::handleRecursiveTailCall(callee, registerOffset, argumentCountIncludingThis, insertChecksWithAccounting)) {
+    if (kind == InlineCallFrame::TailCall && ByteCodeParser::handleRecursiveTailCall(callTargetNode, callee, registerOffset, argumentCountIncludingThis, insertChecksWithAccounting)) {
         RELEASE_ASSERT(didInsertChecks);
         return CallOptimizationResult::OptimizedToJump;
     }
index dd68a59..8834353 100644 (file)
@@ -695,6 +695,10 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         read(AbstractHeap(Stack, CallFrameSlot::callee));
         def(HeapLocation(StackLoc, AbstractHeap(Stack, CallFrameSlot::callee)), LazyNode(node));
         return;
+
+    case SetCallee:
+        write(AbstractHeap(Stack, CallFrameSlot::callee));
+        return;
         
     case GetArgumentCountIncludingThis: {
         auto heap = AbstractHeap(Stack, remapOperand(node->argumentsInlineCallFrame(), VirtualRegister(CallFrameSlot::argumentCount)));
index 390b4e3..8287fac 100644 (file)
@@ -51,6 +51,7 @@ bool doesGC(Graph& graph, Node* node)
     case Identity:
     case IdentityWithProfile:
     case GetCallee:
+    case SetCallee:
     case GetArgumentCountIncludingThis:
     case SetArgumentCountIncludingThis:
     case GetRestLength:
index 47d0e66..dd95388 100644 (file)
@@ -2134,6 +2134,10 @@ private:
             }
             break;
 
+        case SetCallee:
+            fixEdge<CellUse>(node->child1());
+            break;
+
 #if !ASSERT_DISABLED
         // Have these no-op cases here to ensure that nobody forgets to add handlers for new opcodes.
         case SetArgument:
index 159a9bb..d41d1a6 100644 (file)
@@ -74,6 +74,7 @@ ExitMode mayExitImpl(Graph& graph, Node* node, StateType& state)
     case KillStack:
     case GetStack:
     case GetCallee:
+    case SetCallee:
     case GetArgumentCountIncludingThis:
     case SetArgumentCountIncludingThis:
     case GetRestLength:
index d630b1d..075b8af 100644 (file)
@@ -54,6 +54,7 @@ namespace JSC { namespace DFG {
     macro(ToThis, NodeResultJS) \
     macro(CreateThis, NodeResultJS) /* Note this is not MustGenerate since we're returning it anyway. */ \
     macro(GetCallee, NodeResultJS) \
+    macro(SetCallee, NodeMustGenerate) \
     macro(GetArgumentCountIncludingThis, NodeResultInt32) \
     macro(SetArgumentCountIncludingThis, NodeMustGenerate) \
     \
index 4774385..d4bbb4a 100644 (file)
@@ -776,6 +776,7 @@ private:
             break;
         }
 
+        case SetCallee:
         case SetArgumentCountIncludingThis:
             break;
 
index 6bc7451..21db340 100644 (file)
@@ -172,6 +172,7 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node)
     case ToThis:
     case CreateThis:
     case GetCallee:
+    case SetCallee:
     case GetArgumentCountIncludingThis:
     case SetArgumentCountIncludingThis:
     case GetRestLength:
index b095268..92784e8 100644 (file)
@@ -11866,6 +11866,13 @@ void SpeculativeJIT::compileGetCallee(Node* node)
     cellResult(result.gpr(), node);
 }
 
+void SpeculativeJIT::compileSetCallee(Node* node)
+{
+    SpeculateCellOperand callee(this, node->child1());
+    m_jit.storeCell(callee.gpr(), JITCompiler::payloadFor(CallFrameSlot::callee));
+    noResult(node);
+}
+
 void SpeculativeJIT::compileGetArgumentCountIncludingThis(Node* node)
 {
     GPRTemporary result(this);
index bbf174c..bb93965 100644 (file)
@@ -1459,6 +1459,7 @@ public:
     void compileGetGetter(Node*);
     void compileGetSetter(Node*);
     void compileGetCallee(Node*);
+    void compileSetCallee(Node*);
     void compileGetArgumentCountIncludingThis(Node*);
     void compileSetArgumentCountIncludingThis(Node*);
     void compileStrCat(Node*);
index 99bfa09..945e40c 100644 (file)
@@ -3255,6 +3255,11 @@ void SpeculativeJIT::compile(Node* node)
         compileGetCallee(node);
         break;
     }
+
+    case SetCallee: {
+        compileSetCallee(node);
+        break;
+    }
         
     case GetArgumentCountIncludingThis: {
         compileGetArgumentCountIncludingThis(node);
index 5c980e4..2af307f 100644 (file)
@@ -3476,6 +3476,11 @@ void SpeculativeJIT::compile(Node* node)
         compileGetCallee(node);
         break;
     }
+
+    case SetCallee: {
+        compileSetCallee(node);
+        break;
+    }
         
     case GetArgumentCountIncludingThis: {
         compileGetArgumentCountIncludingThis(node);
index f1a45a8..2f74bae 100644 (file)
@@ -182,6 +182,7 @@ inline CapabilityLevel canCompile(Node* node)
     case GetExecutable:
     case GetScope:
     case GetCallee:
+    case SetCallee:
     case GetArgumentCountIncludingThis:
     case SetArgumentCountIncludingThis:
     case ToNumber:
index 3372051..eef68b9 100644 (file)
@@ -924,6 +924,9 @@ private:
         case GetCallee:
             compileGetCallee();
             break;
+        case SetCallee:
+            compileSetCallee();
+            break;
         case GetArgumentCountIncludingThis:
             compileGetArgumentCountIncludingThis();
             break;
@@ -6626,6 +6629,12 @@ private:
     {
         setJSValue(m_out.loadPtr(addressFor(CallFrameSlot::callee)));
     }
+
+    void compileSetCallee()
+    {
+        auto callee = lowCell(m_node->child1());
+        m_out.storePtr(callee, payloadFor(CallFrameSlot::callee));
+    }
     
     void compileGetArgumentCountIncludingThis()
     {