[JSC] Handle PhantomSpread in LoadVarargs as the same to the others

https://bugs.webkit.org/show_bug.cgi?id=171262

Reviewed by Saam Barati.

JSTests:

* stress/spread-outer-create-rest.js: Added.
(assert):
(foo):
(bar):
(baz):

Source/JavaScriptCore:

This is follow-up patch after r215720. In that patch, accidentally
we did not apply the same change to LoadVarargs in argument elimination
phase. This patch just does the same rewriting to handle PhantomSpread
correctly.

* dfg/DFGArgumentsEliminationPhase.cpp:

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

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index e8fe97d..078ed24 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,16 @@
+2017-04-27  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Handle PhantomSpread in LoadVarargs as the same to the others
+        https://bugs.webkit.org/show_bug.cgi?id=171262
+
+        Reviewed by Saam Barati.
+
+        * stress/spread-outer-create-rest.js: Added.
+        (assert):
+        (foo):
+        (bar):
+        (baz):
+
 2017-04-26  Saam Barati  <sbarati@apple.com>
 
         Print Wasm function index in stack trace

diff --git a/JSTests/stress/spread-outer-create-rest.js b/JSTests/stress/spread-outer-create-rest.js
new file mode 100644 (file)
index 0000000..11e881a
--- /dev/null
+++ b/JSTests/stress/spread-outer-create-rest.js
@@ -0,0 +1,22 @@
+"use strict";
+
+function assert(b) {
+    if (!b)
+        throw new Error("Bad");
+}
+
+function foo(...args) {
+    return bar(args);
+}
+noInline(foo);
+
+function bar(args) {
+    return baz(...args);
+}
+
+function baz(a, b) {
+    return a + b;
+}
+
+for (let i = 0; i < 10000; ++i)
+    assert(foo(i, i+1) === (i + (i + 1)));

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 3ee5dd4..17bcc2d 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,17 @@
+2017-04-27  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Handle PhantomSpread in LoadVarargs as the same to the others
+        https://bugs.webkit.org/show_bug.cgi?id=171262
+
+        Reviewed by Saam Barati.
+
+        This is follow-up patch after r215720. In that patch, accidentally
+        we did not apply the same change to LoadVarargs in argument elimination
+        phase. This patch just does the same rewriting to handle PhantomSpread
+        correctly.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+
 2017-04-26  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Uint8ClampedArray should be treated like an array, not an object

diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
index 4663080..673920e 100644 (file)
--- a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
@@ -770,41 +770,52 @@ private:
                             OpInfo(data), Edge(value));
                     };
 
-                    if (candidate->op() == PhantomNewArrayWithSpread) {
+                    if (candidate->op() == PhantomNewArrayWithSpread || candidate->op() == PhantomSpread) {
                         bool canConvertToStaticLoadStores = true;
-                        BitVector* bitVector = candidate->bitVector();
 
-                        for (unsigned i = 0; i < candidate->numChildren(); i++) {
-                            if (bitVector->get(i)) {
-                                Node* child = m_graph.varArgChild(candidate, i).node();
-                                ASSERT(child->op() == PhantomSpread && child->child1()->op() == PhantomCreateRest);
-                                InlineCallFrame* inlineCallFrame = child->child1()->origin.semantic.inlineCallFrame;
-                                if (!inlineCallFrame || inlineCallFrame->isVarargs()) {
-                                    canConvertToStaticLoadStores = false;
-                                    break;
+                        auto canConvertToStaticLoadStoresForSpread = [] (Node* spread) {
+                            ASSERT(spread->op() == PhantomSpread);
+                            ASSERT(spread->child1()->op() == PhantomCreateRest);
+                            InlineCallFrame* inlineCallFrame = spread->child1()->origin.semantic.inlineCallFrame;
+                            return inlineCallFrame && !inlineCallFrame->isVarargs();
+                        };
+
+                        if (candidate->op() == PhantomNewArrayWithSpread) {
+                            BitVector* bitVector = candidate->bitVector();
+                            for (unsigned i = 0; i < candidate->numChildren(); i++) {
+                                if (bitVector->get(i)) {
+                                    if (!canConvertToStaticLoadStoresForSpread(m_graph.varArgChild(candidate, i).node())) {
+                                        canConvertToStaticLoadStores = false;
+                                        break;
+                                    }
                                 }
                             }
-                        }
+                        } else
+                            canConvertToStaticLoadStores = canConvertToStaticLoadStoresForSpread(candidate);
 
                         if (canConvertToStaticLoadStores) {
                             unsigned argumentCountIncludingThis = 1; // |this|
-                            for (unsigned i = 0; i < candidate->numChildren(); i++) {
-                                if (bitVector->get(i)) {
-                                    Node* child = m_graph.varArgChild(candidate, i).node();
-                                    ASSERT(child->op() == PhantomSpread && child->child1()->op() == PhantomCreateRest);
-                                    unsigned numberOfArgumentsToSkip = child->child1()->numberOfArgumentsToSkip();
-                                    InlineCallFrame* inlineCallFrame = child->child1()->origin.semantic.inlineCallFrame;
-                                    unsigned numberOfSpreadArguments;
-                                    unsigned frameArgumentCount = inlineCallFrame->arguments.size() - 1;
-                                    if (frameArgumentCount >= numberOfArgumentsToSkip)
-                                        numberOfSpreadArguments = frameArgumentCount - numberOfArgumentsToSkip;
-                                    else
-                                        numberOfSpreadArguments = 0;
 
-                                    argumentCountIncludingThis += numberOfSpreadArguments;
-                                } else
-                                    ++argumentCountIncludingThis;
-                            }
+                            auto countNumberOfSpreadArguments = [] (Node* spread) -> unsigned {
+                                ASSERT(spread->op() == PhantomSpread && spread->child1()->op() == PhantomCreateRest);
+                                unsigned numberOfArgumentsToSkip = spread->child1()->numberOfArgumentsToSkip();
+                                InlineCallFrame* inlineCallFrame = spread->child1()->origin.semantic.inlineCallFrame;
+                                unsigned frameArgumentCount = inlineCallFrame->arguments.size() - 1;
+                                if (frameArgumentCount >= numberOfArgumentsToSkip)
+                                    return frameArgumentCount - numberOfArgumentsToSkip;
+                                return 0;
+                            };
+
+                            if (candidate->op() == PhantomNewArrayWithSpread) {
+                                BitVector* bitVector = candidate->bitVector();
+                                for (unsigned i = 0; i < candidate->numChildren(); i++) {
+                                    if (bitVector->get(i))
+                                        argumentCountIncludingThis += countNumberOfSpreadArguments(m_graph.varArgChild(candidate, i).node());
+                                    else
+                                        ++argumentCountIncludingThis;
+                                }
+                            } else
+                                argumentCountIncludingThis += countNumberOfSpreadArguments(candidate);
 
                             if (argumentCountIncludingThis <= varargsData->limit) {
                                 storeArgumentCountIncludingThis(argumentCountIncludingThis);
@@ -813,28 +824,37 @@ private:
                                 // Define our limit to exclude "this", since that's a bit easier to reason about.
                                 unsigned limit = varargsData->limit - 1;
                                 unsigned storeIndex = 0;
-                                for (unsigned i = 0; i < candidate->numChildren(); i++) {
-                                    if (bitVector->get(i)) {
-                                        Node* child = m_graph.varArgChild(candidate, i).node();
-                                        ASSERT(child->op() == PhantomSpread && child->child1()->op() == PhantomCreateRest);
-                                        unsigned numberOfArgumentsToSkip = child->child1()->numberOfArgumentsToSkip();
-                                        InlineCallFrame* inlineCallFrame = child->child1()->origin.semantic.inlineCallFrame;
-                                        unsigned frameArgumentCount = inlineCallFrame->arguments.size() - 1;
-                                        for (unsigned loadIndex = numberOfArgumentsToSkip; loadIndex < frameArgumentCount; ++loadIndex) {
-                                            VirtualRegister reg = virtualRegisterForArgument(loadIndex + 1) + inlineCallFrame->stackOffset;
-                                            StackAccessData* data = m_graph.m_stackAccessData.add(reg, FlushedJSValue);
-                                            Node* value = insertionSet.insertNode(
-                                                nodeIndex, SpecNone, GetStack, node->origin.withExitOK(canExit),
-                                                OpInfo(data));
+
+                                auto forwardSpread = [&] (Node* spread, unsigned storeIndex) -> unsigned {
+                                    ASSERT(spread->op() == PhantomSpread && spread->child1()->op() == PhantomCreateRest);
+                                    unsigned numberOfArgumentsToSkip = spread->child1()->numberOfArgumentsToSkip();
+                                    InlineCallFrame* inlineCallFrame = spread->child1()->origin.semantic.inlineCallFrame;
+                                    unsigned frameArgumentCount = inlineCallFrame->arguments.size() - 1;
+                                    for (unsigned loadIndex = numberOfArgumentsToSkip; loadIndex < frameArgumentCount; ++loadIndex) {
+                                        VirtualRegister reg = virtualRegisterForArgument(loadIndex + 1) + inlineCallFrame->stackOffset;
+                                        StackAccessData* data = m_graph.m_stackAccessData.add(reg, FlushedJSValue);
+                                        Node* value = insertionSet.insertNode(
+                                            nodeIndex, SpecNone, GetStack, node->origin.withExitOK(canExit),
+                                            OpInfo(data));
+                                        storeValue(value, storeIndex);
+                                        ++storeIndex;
+                                    }
+                                    return storeIndex;
+                                };
+
+                                if (candidate->op() == PhantomNewArrayWithSpread) {
+                                    BitVector* bitVector = candidate->bitVector();
+                                    for (unsigned i = 0; i < candidate->numChildren(); i++) {
+                                        if (bitVector->get(i))
+                                            storeIndex = forwardSpread(m_graph.varArgChild(candidate, i).node(), storeIndex);
+                                        else {
+                                            Node* value = m_graph.varArgChild(candidate, i).node();
                                             storeValue(value, storeIndex);
                                             ++storeIndex;
                                         }
-                                    } else {
-                                        Node* value = m_graph.varArgChild(candidate, i).node();
-                                        storeValue(value, storeIndex);
-                                        ++storeIndex;
                                     }
-                                }
+                                } else
+                                    storeIndex = forwardSpread(candidate, storeIndex);
 
                                 RELEASE_ASSERT(storeIndex <= limit);
                                 Node* undefined = nullptr;
@@ -855,10 +875,6 @@ private:
                         unsigned numberOfArgumentsToSkip = 0;
                         if (candidate->op() == PhantomCreateRest)
                             numberOfArgumentsToSkip = candidate->numberOfArgumentsToSkip();
-                        else if (candidate->op() == PhantomSpread) {
-                            ASSERT(candidate->child1()->op() == PhantomCreateRest);
-                            numberOfArgumentsToSkip = candidate->child1()->numberOfArgumentsToSkip();
-                        }
                         varargsData->offset += numberOfArgumentsToSkip;
 
                         InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame;