Attachment 371309 Details for Bug 198520 – Patch for landing

[patch] Patch for landing

bug-198520-20190604202455.patch (text/plain), 13.38 KB, created by Tadeu Zagallo on 2019-06-04 11:24:57 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-06-04 11:24:57 PDT

Size: 13.38 KB

patch

obsolete

>Subversion Revision: 246071
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 40a5487c77246e7a414409db0986cd5c757d17b6..159a6935bd81b523bc2a9f5259f683e9772ba8a9 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        Argument elimination should check transitive dependents for interference
>+        https://bugs.webkit.org/show_bug.cgi?id=198520
>+        <rdar://problem/50863343>
>+
>+        Reviewed by Filip Pizlo.
>+
>+        Consider the following program:
>+
>+            a: CreateRest
>+            -->
>+                b: CreateRest
>+            <--
>+            c: Spread(@a)
>+            d: Spread(@b)
>+            e: NewArrayWithSpread(@a, @b)
>+            f: KillStack(locX)
>+            g: LoadVarargs(@e)
>+
>+        Suppose @b reads locX, then we cannot transform @e to PhantomNewArraySpread, since that would
>+        move the stack access from @b into @g, and that stack location is no longer valid at that point.
>+
>+        We fix that by computing a set of all inline call frames that any argument elimination candidate
>+        depends on and checking each of them for interference in `eliminateCandidatesThatInterfere`.
>+
>+        * dfg/DFGArgumentsEliminationPhase.cpp:
>+
> 2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         Argument elimination should check for negative indices in GetByVal
>diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>index b7e4a4fe2c1f6d4cc06a8dd01e528a31ce8a7891..21b82917ffb39b367ff0aaeda8a21615c6fc92c9 100644
>--- a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>@@ -502,6 +502,33 @@ private:
>             }
>         }
>         
>+        using InlineCallFrames = HashSet<InlineCallFrame*, WTF::DefaultHash<InlineCallFrame*>::Hash, WTF::NullableHashTraits<InlineCallFrame*>>;
>+        using InlineCallFramesForCanditates = HashMap<Node*, InlineCallFrames>;
>+        InlineCallFramesForCanditates inlineCallFramesForCandidate;
>+        auto forEachDependentNode = recursableLambda([&](auto self, Node* node, const auto& functor) -> void {
>+            functor(node);
>+
>+            if (node->op() == Spread) {
>+                self(node->child1().node(), functor);
>+                return;
>+            }
>+
>+            if (node->op() == NewArrayWithSpread) {
>+                BitVector* bitVector = node->bitVector();
>+                for (unsigned i = node->numChildren(); i--; ) {
>+                    if (bitVector->get(i))
>+                        self(m_graph.varArgChild(node, i).node(), functor);
>+                }
>+                return;
>+            }
>+        });
>+        for (Node* candidate : m_candidates) {
>+            auto& set = inlineCallFramesForCandidate.add(candidate, InlineCallFrames()).iterator->value;
>+            forEachDependentNode(candidate, [&](Node* dependent) {
>+                set.add(dependent->origin.semantic.inlineCallFrame());
>+            });
>+        }
>+
>         for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
>             // Stop if we've already removed all candidates.
>             if (m_candidates.isEmpty())
>@@ -524,83 +551,85 @@ private:
>                     if (!m_candidates.contains(candidate))
>                         return;
>                     
>-                    // Check if this block has any clobbers that affect this candidate. This is a fairly
>-                    // fast check.
>-                    bool isClobberedByBlock = false;
>-                    Operands<bool>& clobberedByThisBlock = clobberedByBlock[block];
>-                    
>-                    if (InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame()) {
>-                        if (inlineCallFrame->isVarargs()) {
>-                            isClobberedByBlock |= clobberedByThisBlock.operand(
>-                                inlineCallFrame->stackOffset + CallFrameSlot::argumentCount);
>-                        }
>-                        
>-                        if (!isClobberedByBlock || inlineCallFrame->isClosureCall) {
>-                            isClobberedByBlock |= clobberedByThisBlock.operand(
>-                                inlineCallFrame->stackOffset + CallFrameSlot::callee);
>-                        }
>+                    for (InlineCallFrame* inlineCallFrame : inlineCallFramesForCandidate.get(candidate)) {
>+                        // Check if this block has any clobbers that affect this candidate. This is a fairly
>+                        // fast check.
>+                        bool isClobberedByBlock = false;
>+                        Operands<bool>& clobberedByThisBlock = clobberedByBlock[block];
>                         
>-                        if (!isClobberedByBlock) {
>-                            for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) {
>-                                VirtualRegister reg =
>-                                    VirtualRegister(inlineCallFrame->stackOffset) +
>-                                    CallFrame::argumentOffset(i);
>-                                if (clobberedByThisBlock.operand(reg)) {
>+                        if (inlineCallFrame) {
>+                            if (inlineCallFrame->isVarargs()) {
>+                                isClobberedByBlock |= clobberedByThisBlock.operand(
>+                                    inlineCallFrame->stackOffset + CallFrameSlot::argumentCount);
>+                            }
>+
>+                            if (!isClobberedByBlock || inlineCallFrame->isClosureCall) {
>+                                isClobberedByBlock |= clobberedByThisBlock.operand(
>+                                    inlineCallFrame->stackOffset + CallFrameSlot::callee);
>+                            }
>+
>+                            if (!isClobberedByBlock) {
>+                                for (unsigned i = 0; i < inlineCallFrame->argumentCountIncludingThis - 1; ++i) {
>+                                    VirtualRegister reg =
>+                                        VirtualRegister(inlineCallFrame->stackOffset) +
>+                                        CallFrame::argumentOffset(i);
>+                                    if (clobberedByThisBlock.operand(reg)) {
>+                                        isClobberedByBlock = true;
>+                                        break;
>+                                    }
>+                                }
>+                            }
>+                        } else {
>+                            // We don't include the ArgumentCount or Callee in this case because we can be
>+                            // damn sure that this won't be clobbered.
>+                            for (unsigned i = 1; i < static_cast<unsigned>(codeBlock()->numParameters()); ++i) {
>+                                if (clobberedByThisBlock.argument(i)) {
>                                     isClobberedByBlock = true;
>                                     break;
>                                 }
>                             }
>                         }
>-                    } else {
>-                        // We don't include the ArgumentCount or Callee in this case because we can be
>-                        // damn sure that this won't be clobbered.
>-                        for (unsigned i = 1; i < static_cast<unsigned>(codeBlock()->numParameters()); ++i) {
>-                            if (clobberedByThisBlock.argument(i)) {
>-                                isClobberedByBlock = true;
>-                                break;
>-                            }
>-                        }
>-                    }
>-                    
>-                    if (!isClobberedByBlock)
>-                        return;
>-                    
>-                    // Check if we can immediately eliminate this candidate. If the block has a clobber
>-                    // for this arguments allocation, and we'd have to examine every node in the block,
>-                    // then we can just eliminate the candidate.
>-                    if (nodeIndex == block->size() && candidate->owner != block) {
>-                        if (DFGArgumentsEliminationPhaseInternal::verbose)
>-                            dataLog("eliminating candidate: ", candidate, " because it is clobbered by: ", block->at(nodeIndex), "\n");
>-                        transitivelyRemoveCandidate(candidate);
>-                        return;
>-                    }
>-                    
>-                    // This loop considers all nodes up to the nodeIndex, excluding the nodeIndex.
>-                    while (nodeIndex--) {
>-                        Node* node = block->at(nodeIndex);
>-                        if (node == candidate)
>-                            break;
>                         
>-                        bool found = false;
>-                        clobberize(
>-                            m_graph, node, NoOpClobberize(),
>-                            [&] (AbstractHeap heap) {
>-                                if (heap.kind() == Stack && !heap.payload().isTop()) {
>-                                    if (argumentsInvolveStackSlot(candidate, VirtualRegister(heap.payload().value32())))
>-                                        found = true;
>-                                    return;
>-                                }
>-                                if (heap.overlaps(Stack))
>-                                    found = true;
>-                            },
>-                            NoOpClobberize());
>+                        if (!isClobberedByBlock)
>+                            continue;
>                         
>-                        if (found) {
>+                        // Check if we can immediately eliminate this candidate. If the block has a clobber
>+                        // for this arguments allocation, and we'd have to examine every node in the block,
>+                        // then we can just eliminate the candidate.
>+                        if (nodeIndex == block->size() && candidate->owner != block) {
>                             if (DFGArgumentsEliminationPhaseInternal::verbose)
>-                                dataLog("eliminating candidate: ", candidate, " because it is clobbered by ", block->at(nodeIndex), "\n");
>+                                dataLog("eliminating candidate: ", candidate, " because it is clobbered by: ", block->at(nodeIndex), "\n");
>                             transitivelyRemoveCandidate(candidate);
>                             return;
>                         }
>+
>+                        // This loop considers all nodes up to the nodeIndex, excluding the nodeIndex.
>+                        while (nodeIndex--) {
>+                            Node* node = block->at(nodeIndex);
>+                            if (node == candidate)
>+                                break;
>+
>+                            bool found = false;
>+                            clobberize(
>+                                m_graph, node, NoOpClobberize(),
>+                                [&] (AbstractHeap heap) {
>+                                    if (heap.kind() == Stack && !heap.payload().isTop()) {
>+                                        if (argumentsInvolveStackSlot(inlineCallFrame, VirtualRegister(heap.payload().value32())))
>+                                            found = true;
>+                                        return;
>+                                    }
>+                                    if (heap.overlaps(Stack))
>+                                        found = true;
>+                                },
>+                                NoOpClobberize());
>+
>+                            if (found) {
>+                                if (DFGArgumentsEliminationPhaseInternal::verbose)
>+                                    dataLog("eliminating candidate: ", candidate, " because it is clobbered by ", block->at(nodeIndex), "\n");
>+                                transitivelyRemoveCandidate(candidate);
>+                                return;
>+                            }
>+                        }
>                     }
>                 });
>         }
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index c42825b2715ae7896ab5561ab0e1a9f4caf644ec..87b3b1924b383aab67020d4fc8998c2b00786810 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        Argument elimination should check transitive dependents for interference
>+        https://bugs.webkit.org/show_bug.cgi?id=198520
>+        <rdar://problem/50863343>
>+
>+        Reviewed by Filip Pizlo.
>+
>+        * stress/argument-elimination-inline-rest-past-kill.js: Added.
>+        (f2):
>+        (f3):
>+
> 2019-06-04  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         Argument elimination should check for negative indices in GetByVal
>diff --git a/JSTests/stress/argument-elimination-inline-rest-past-kill.js b/JSTests/stress/argument-elimination-inline-rest-past-kill.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..69a521fb3e66f16b6872541e177fe36484e9f047
>--- /dev/null
>+++ b/JSTests/stress/argument-elimination-inline-rest-past-kill.js
>@@ -0,0 +1,16 @@
>+//@ requireOptions("--forceEagerCompilation=1")
>+function f2(...a1) {
>+    return a1;
>+}
>+
>+function f3(...a2) {
>+    let v1 = f2([]);
>+    return f2(...a2, ...v1);
>+}
>+noInline(f3);
>+
>+for (let i = 0; i < 1e5; i++) {
>+    var v3 = f3();
>+    if (!Array.isArray(v3[v3.length - 1]))
>+        throw new Error('Should be an array');
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 198520: 371256 | 371309