ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Sep 2018 20:12:26 +0000 (20:12 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Sep 2018 20:12:26 +0000 (20:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189682
<rdar://problem/43557315>

Reviewed by Mark Lam.

JSTests:

* stress/arguments-elimination-will-generate-edge-without-result.js: Added.
(foo):

Source/JavaScriptCore:

Otherwise, if we have code like this:
```
a: Arguments
b: GetButterfly(@a)
c: ForceExit
d: GetArrayLength(@a, @b)
```
it will get transformed into this invalid DFG IR:
```
a: PhantomArguments
b: Check(@a)
c: ForceExit
d: GetArrayLength(@a, @b)
```

And we will fail DFG validation since @b does not have a result.

The fix is to just remove all nodes after the ForceExit and plant an
Unreachable after it. So the above code program will now turn into this:
```
a: PhantomArguments
b: Check(@a)
c: ForceExit
e: Unreachable
```

* dfg/DFGArgumentsEliminationPhase.cpp:

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

JSTests/ChangeLog
JSTests/stress/arguments-elimination-will-generate-edge-without-result.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp

index a975855..06b9ff8 100644 (file)
@@ -1,3 +1,14 @@
+2018-09-24  Saam barati  <sbarati@apple.com>
+
+        ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
+        https://bugs.webkit.org/show_bug.cgi?id=189682
+        <rdar://problem/43557315>
+
+        Reviewed by Mark Lam.
+
+        * stress/arguments-elimination-will-generate-edge-without-result.js: Added.
+        (foo):
+
 2018-09-22  Saam barati  <sbarati@apple.com>
 
         The sampling should not use Strong<CodeBlock> in its machineLocation field
diff --git a/JSTests/stress/arguments-elimination-will-generate-edge-without-result.js b/JSTests/stress/arguments-elimination-will-generate-edge-without-result.js
new file mode 100644 (file)
index 0000000..49bad35
--- /dev/null
@@ -0,0 +1,9 @@
+//@ runDefault("--validateGraphAtEachPhase=true", "--jitPolicyScale=0", "--useConcurrentJIT=0")
+
+'use strict';
+function foo() {
+  return arguments[1][0] === arguments[0];
+}
+for (let i = 0; i < 100000; ++i) {
+  foo(0, 0);
+}
index 08a5244..60770a4 100644 (file)
@@ -1,3 +1,39 @@
+2018-09-24  Saam barati  <sbarati@apple.com>
+
+        ArgumentsEliminationPhase should snip basic blocks after proven OSR exits
+        https://bugs.webkit.org/show_bug.cgi?id=189682
+        <rdar://problem/43557315>
+
+        Reviewed by Mark Lam.
+
+        Otherwise, if we have code like this:
+        ```
+        a: Arguments
+        b: GetButterfly(@a)
+        c: ForceExit
+        d: GetArrayLength(@a, @b)
+        ```
+        it will get transformed into this invalid DFG IR:
+        ```
+        a: PhantomArguments
+        b: Check(@a)
+        c: ForceExit
+        d: GetArrayLength(@a, @b)
+        ```
+        
+        And we will fail DFG validation since @b does not have a result.
+        
+        The fix is to just remove all nodes after the ForceExit and plant an
+        Unreachable after it. So the above code program will now turn into this:
+        ```
+        a: PhantomArguments
+        b: Check(@a)
+        c: ForceExit
+        e: Unreachable
+        ```
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+
 2018-09-22  Saam barati  <sbarati@apple.com>
 
         The sampling should not use Strong<CodeBlock> in its machineLocation field
index ed35269..5aaa02d 100644 (file)
@@ -624,9 +624,12 @@ private:
     
     void transform()
     {
-        InsertionSet insertionSet(m_graph);
+        bool modifiedCFG = false;
         
+        InsertionSet insertionSet(m_graph);
+
         for (BasicBlock* block : m_graph.blocksInPreOrder()) {
+            Node* pseudoTerminal = nullptr;
             for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
                 Node* node = block->at(nodeIndex);
                 
@@ -1210,11 +1213,32 @@ private:
                 default:
                     break;
                 }
-                if (node->isPseudoTerminal())
+
+                if (node->isPseudoTerminal()) {
+                    pseudoTerminal = node;
                     break;
+                }
             }
-            
+
             insertionSet.execute(block);
+
+            if (pseudoTerminal) {
+                for (unsigned i = 0; i < block->size(); ++i) {
+                    Node* node = block->at(i);
+                    if (node != pseudoTerminal)
+                        continue;
+                    block->resize(i + 1);
+                    block->append(m_graph.addNode(SpecNone, Unreachable, node->origin));
+                    modifiedCFG = true;
+                    break;
+                }
+            }
+        }
+
+        if (modifiedCFG) {
+            m_graph.invalidateCFG();
+            m_graph.resetReachability();
+            m_graph.killUnreachableBlocks();
         }
     }