Attachment 358753 Details for Bug 193304 – patch

[patch] patch

c-backup.diff (text/plain), 6.77 KB, created by Saam Barati on 2019-01-09 15:05:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-01-09 15:05:36 PST

Size: 6.77 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 239787)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-01-09  Saam Barati  <sbarati@apple.com>
>+
>+        DFG combined liveness can be wrong for terminal basic blocks
>+        https://bugs.webkit.org/show_bug.cgi?id=193304
>+        <rdar://problem/45268632>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/dfg-combined-liveness-consider-terminal-blocks-bytecode-liveness.js: Added.
>+        (foo.args1):
>+        (foo.args2):
>+        (foo):
>+
> 2019-01-08  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Array.prototype.flat/flatMap have a minor bug in ArraySpeciesCreate
>Index: JSTests/stress/dfg-combined-liveness-consider-terminal-blocks-bytecode-liveness.js
>===================================================================
>--- JSTests/stress/dfg-combined-liveness-consider-terminal-blocks-bytecode-liveness.js	(nonexistent)
>+++ JSTests/stress/dfg-combined-liveness-consider-terminal-blocks-bytecode-liveness.js	(working copy)
>@@ -0,0 +1,19 @@
>+//@ runDefault("--useConcurrentJIT=0", "--usePutStackSinking=0")
>+
>+function foo() {
>+    var args1 = function () {
>+        return arguments;
>+    }();
>+    var args2 = function () {
>+        var result = arguments;
>+        result.length = 1;
>+        return result;
>+    }(1);
>+    for (var i = 0; i < 10000000; ++i) {
>+        args1.length;
>+        args2.length;
>+    }
>+}
>+foo();
>+foo();
>+foo();
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 239761)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,36 @@
>+2019-01-09  Saam Barati  <sbarati@apple.com>
>+
>+        DFG combined liveness can be wrong for terminal basic blocks
>+        https://bugs.webkit.org/show_bug.cgi?id=193304
>+        <rdar://problem/45268632>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If a block doesn't have any successors, it can't rely on the typical
>+        backwards liveness propagation that CombinedLiveness was doing. The phase
>+        first got what was live in bytecode and IR at the heads of each block. Then
>+        for each block, it made the live at tail the union of the live at head for
>+        each successor. For a terminal block though, this could be wrong. We could
>+        end up saying nothing is live even though many things may be live in bytecode.
>+        We must account for what's bytecode live at the end of the block. Consider a
>+        block that ends with:
>+        ```
>+        ForceOSRExit
>+        Unreachable
>+        ```
>+        
>+        Things may definitely be live in bytecode at the tail. However, we'll
>+        report nothing as being alive. This probably subtly breaks many analyses,
>+        but we have a test case of it breaking the interference analysis that
>+        the ArgumentsEliminationPhase performs.
>+
>+        * dfg/DFGBasicBlock.h:
>+        (JSC::DFG::BasicBlock::last const):
>+        * dfg/DFGCombinedLiveness.cpp:
>+        (JSC::DFG::addBytecodeLiveness):
>+        (JSC::DFG::liveNodesAtHead):
>+        (JSC::DFG::CombinedLiveness::CombinedLiveness):
>+
> 2019-01-08  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Array.prototype.flat/flatMap have a minor bug in ArraySpeciesCreate
>Index: Source/JavaScriptCore/dfg/DFGBasicBlock.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGBasicBlock.h	(revision 239761)
>+++ Source/JavaScriptCore/dfg/DFGBasicBlock.h	(working copy)
>@@ -64,6 +64,11 @@ struct BasicBlock : RefCounted<BasicBloc
>     }
>     Node*& operator[](size_t i) { return at(i); }
>     Node* operator[](size_t i) const { return at(i); }
>+    Node* last() const
>+    {
>+        RELEASE_ASSERT(!!size());
>+        return at(size() - 1);
>+    }
>     
>     // Use this to find both the index of the terminal and the terminal itself in one go. May
>     // return a clear NodeAndIndex if the basic block currently lacks a terminal. That may happen
>Index: Source/JavaScriptCore/dfg/DFGCombinedLiveness.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGCombinedLiveness.cpp	(revision 239761)
>+++ Source/JavaScriptCore/dfg/DFGCombinedLiveness.cpp	(working copy)
>@@ -35,17 +35,10 @@
> 
> namespace JSC { namespace DFG {
> 
>-NodeSet liveNodesAtHead(Graph& graph, BasicBlock* block)
>+static void addBytecodeLiveness(Graph& graph, AvailabilityMap& availabilityMap, NodeSet& seen, Node* node)
> {
>-    NodeSet seen;
>-    for (NodeFlowProjection node : block->ssa->liveAtHead) {
>-        if (node.kind() == NodeFlowProjection::Primary)
>-            seen.addVoid(node.node());
>-    }
>-    
>-    AvailabilityMap& availabilityMap = block->ssa->availabilityAtHead;
>     graph.forAllLiveInBytecode(
>-        block->at(0)->origin.forExit,
>+        node->origin.forExit,
>         [&] (VirtualRegister reg) {
>             availabilityMap.closeStartingWithLocal(
>                 reg,
>@@ -56,7 +49,17 @@ NodeSet liveNodesAtHead(Graph& graph, Ba
>                     return seen.add(node).isNewEntry;
>                 });
>         });
>-    
>+}
>+
>+NodeSet liveNodesAtHead(Graph& graph, BasicBlock* block)
>+{
>+    NodeSet seen;
>+    for (NodeFlowProjection node : block->ssa->liveAtHead) {
>+        if (node.kind() == NodeFlowProjection::Primary)
>+            seen.addVoid(node.node());
>+    }
>+
>+    addBytecodeLiveness(graph, block->ssa->availabilityAtHead, seen, block->at(0));
>     return seen;
> }
> 
>@@ -64,9 +67,27 @@ CombinedLiveness::CombinedLiveness(Graph
>     : liveAtHead(graph)
>     , liveAtTail(graph)
> {
>-    // First compute the liveAtHead for each block.
>-    for (BasicBlock* block : graph.blocksInNaturalOrder())
>+    // First compute 
>+    // - The liveAtHead for each block.
>+    // - The liveAtTail for blocks that won't properly propagate
>+    //   the information based on their empty successor list.
>+    for (BasicBlock* block : graph.blocksInNaturalOrder()) {
>         liveAtHead[block] = liveNodesAtHead(graph, block);
>+
>+        // If we don't have successors, we can't rely on the propagation below. This doesn't usually
>+        // do anything for terminal blocks, since the last node is usually a return, so nothing is live
>+        // after it. However, we may also have the end of the basic block be:
>+        //
>+        // ForceOSRExit
>+        // Unreachable
>+        //
>+        // And things may definitely be live in bytecode at that point in the program.
>+        if (!block->numSuccessors()) {
>+            NodeSet seen;
>+            addBytecodeLiveness(graph, block->ssa->availabilityAtTail, seen, block->last());
>+            liveAtTail[block] = seen;
>+        }
>+    }
>     
>     // Now compute the liveAtTail by unifying the liveAtHead of the successors.
>     for (BasicBlock* block : graph.blocksInNaturalOrder()) {

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193304: 358753 | 358955