[FTL] Arguments elimination is suppressed by unreachable blocks
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Jul 2017 16:01:01 +0000 (16:01 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Jul 2017 16:01:01 +0000 (16:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174352

Reviewed by Filip Pizlo.

JSTests:

* stress/arguments-elimination-force-exit.js: Added.
(shouldBe):
(strict):
(sloppy):
* stress/arguments-elimination-throw.js: Added.
(shouldBe):
(shouldThrow):
(sloppy):
(isArguments):

Source/JavaScriptCore:

If we do not execute `op_get_by_id`, our value profiling tells us unpredictable and DFG emits ForceOSRExit.
The problem is that arguments elimination phase checks escaping even when ForceOSRExit preceeds.
Since GetById without information can escape arguments if it is specified, non-executed code including
op_get_by_id with arguments can escape arguments.

For example,

    function test(flag)
    {
        if (flag) {
            // This is not executed, but emits GetById with arguments.
            // It prevents us from eliminating materialization.
            return arguments.length;
        }
        return arguments.length;
    }
    noInline(test);
    while (true)
        test(false);

We do not perform CFA and dead-node clipping yet when performing arguments elimination phase.
So this GetById exists and escapes arguments.

To solve this problem, our arguments elimination phase checks preceding pseudo-terminal nodes.
If it is shown, following GetById does not escape arguments. Compared to performing AI, it is
lightweight. But it catches much of typical cases we failed to perform arguments elimination.

* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGNode.h:
(JSC::DFG::Node::isPseudoTerminal):
* dfg/DFGValidate.cpp:

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

JSTests/ChangeLog
JSTests/stress/arguments-elimination-force-exit.js [new file with mode: 0644]
JSTests/stress/arguments-elimination-throw.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGValidate.cpp

index 06eb3a9..2432553 100644 (file)
@@ -1,3 +1,20 @@
+2017-07-21  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [FTL] Arguments elimination is suppressed by unreachable blocks
+        https://bugs.webkit.org/show_bug.cgi?id=174352
+
+        Reviewed by Filip Pizlo.
+
+        * stress/arguments-elimination-force-exit.js: Added.
+        (shouldBe):
+        (strict):
+        (sloppy):
+        * stress/arguments-elimination-throw.js: Added.
+        (shouldBe):
+        (shouldThrow):
+        (sloppy):
+        (isArguments):
+
 2017-07-13  Mark Lam  <mark.lam@apple.com>
 
         Add some additional test cases for bug 170896.
diff --git a/JSTests/stress/arguments-elimination-force-exit.js b/JSTests/stress/arguments-elimination-force-exit.js
new file mode 100644 (file)
index 0000000..a50b46a
--- /dev/null
@@ -0,0 +1,29 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+function strict(flag)
+{
+    "use strict";
+    if (flag)
+        return arguments.length + 42;
+    return arguments.length;
+}
+noInline(strict);
+
+function sloppy(flag)
+{
+    if (flag)
+        return arguments.length + 42;
+    return arguments.length;
+}
+noInline(sloppy);
+
+for (var i = 0; i < 1e6; ++i) {
+    shouldBe(strict(false), 1);
+    shouldBe(sloppy(false), 1);
+}
+shouldBe(strict(true), 43);
+shouldBe(sloppy(true), 43);
diff --git a/JSTests/stress/arguments-elimination-throw.js b/JSTests/stress/arguments-elimination-throw.js
new file mode 100644 (file)
index 0000000..3b5119a
--- /dev/null
@@ -0,0 +1,51 @@
+function shouldBe(actual, expected)
+{
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+function shouldThrow(func, errorCondition) {
+    var errorThrown = false;
+    var error = null;
+    try {
+        func();
+    } catch (e) {
+        errorThrown = true;
+        error = e;
+    }
+    if (!errorThrown)
+        throw new Error('not thrown');
+    if (!errorCondition(error))
+        throw new Error(`bad error: ${String(error)}`);
+}
+
+function strict(flag)
+{
+    "use strict";
+    if (flag)
+        throw arguments;
+    return arguments.length;
+}
+noInline(strict);
+
+function sloppy(flag)
+{
+    if (flag)
+        throw arguments;
+    return arguments.length;
+}
+noInline(sloppy);
+
+for (var i = 0; i < 1e6; ++i) {
+    shouldBe(strict(false), 1);
+    shouldBe(sloppy(false), 1);
+}
+function isArguments(arg)
+{
+    shouldBe(String(arg), `[object Arguments]`);
+    shouldBe(arg.length, 1);
+    shouldBe(arg[0], true);
+    return true;
+}
+shouldThrow(() => strict(true), isArguments);
+shouldThrow(() => sloppy(true), isArguments);
index 99f8093..e5c440a 100644 (file)
@@ -1,3 +1,42 @@
+2017-07-21  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [FTL] Arguments elimination is suppressed by unreachable blocks
+        https://bugs.webkit.org/show_bug.cgi?id=174352
+
+        Reviewed by Filip Pizlo.
+
+        If we do not execute `op_get_by_id`, our value profiling tells us unpredictable and DFG emits ForceOSRExit.
+        The problem is that arguments elimination phase checks escaping even when ForceOSRExit preceeds.
+        Since GetById without information can escape arguments if it is specified, non-executed code including
+        op_get_by_id with arguments can escape arguments.
+
+        For example,
+
+            function test(flag)
+            {
+                if (flag) {
+                    // This is not executed, but emits GetById with arguments.
+                    // It prevents us from eliminating materialization.
+                    return arguments.length;
+                }
+                return arguments.length;
+            }
+            noInline(test);
+            while (true)
+                test(false);
+
+        We do not perform CFA and dead-node clipping yet when performing arguments elimination phase.
+        So this GetById exists and escapes arguments.
+
+        To solve this problem, our arguments elimination phase checks preceding pseudo-terminal nodes.
+        If it is shown, following GetById does not escape arguments. Compared to performing AI, it is
+        lightweight. But it catches much of typical cases we failed to perform arguments elimination.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::isPseudoTerminal):
+        * dfg/DFGValidate.cpp:
+
 2017-07-20  Chris Dumez  <cdumez@apple.com>
 
         Replace calls to Vector::resize() with calls to more efficient shrink() / grow() when applicable
index 1483818..fd6bf98 100644 (file)
@@ -409,6 +409,8 @@ private:
                     m_graph.doToChildren(node, [&] (Edge edge) { return escape(edge, node); });
                     break;
                 }
+                if (node->isPseudoTerminal())
+                    break;
             }
         }
 
@@ -1101,6 +1103,8 @@ private:
                 default:
                     break;
                 }
+                if (node->isPseudoTerminal())
+                    break;
             }
             
             insertionSet.execute(block);
index 24cecdd..7bed8f9 100644 (file)
@@ -1307,6 +1307,24 @@ public:
         return false;
     }
 
+    // As is described in DFGNodeType.h's ForceOSRExit, this is a pseudo-terminal.
+    // It means that execution should fall out of DFG at this point, but execution
+    // does continue in the basic block - just in a different compiler.
+    // FIXME: This is used for lightweight reachability decision. But this should
+    // be replaced with AI-based reachability ideally.
+    bool isPseudoTerminal()
+    {
+        switch (op()) {
+        case ForceOSRExit:
+        case CheckBadCell:
+        case Throw:
+        case ThrowStaticError:
+            return true;
+        default:
+            return false;
+        }
+    }
+
     unsigned targetBytecodeOffsetDuringParsing()
     {
         ASSERT(isJump());
index 7436adb..20c9a33 100644 (file)
@@ -642,6 +642,7 @@ private:
             VALIDATE((block), block->phis.isEmpty());
 
             bool didSeeExitOK = false;
+            bool isOSRExited = false;
             
             for (auto* node : *block) {
                 didSeeExitOK |= node->origin.exitOK;
@@ -669,6 +670,9 @@ private:
                     // https://bugs.webkit.org/show_bug.cgi?id=123471
                     break;
                 }
+
+                if (isOSRExited)
+                    continue;
                 switch (node->op()) {
                 case PhantomNewObject:
                 case PhantomNewFunction:
@@ -738,6 +742,7 @@ private:
                         });
                     break;
                 }
+                isOSRExited |= node->isPseudoTerminal();
             }
         }
     }