Changeset 246420 in webkit

Timestamp:

Jun 13, 2019, 7:30:42 PM (7 years ago)

Author:

Kocsen Chung

Message:

Apply patch. rdar://problem/51656844

Location:

branches/safari-607-branch

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/argument-elimination-inline-rest-past-kill.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp (modified) (3 diffs)

branches/safari-607-branch/JSTests/ChangeLog

@@ +1 @@
+2019-06-13  Kocsen Chung  <kocsen_chung@apple.com>
+
+        Apply patch. rdar://problem/51656844
+
+    2019-06-13  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-12  Null  <null@apple.com>
 

branches/safari-607-branch/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-06-13  Kocsen Chung  <kocsen_chung@apple.com>
+
+        Apply patch. rdar://problem/51656844
+
+    2019-06-13  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-12  Null  <null@apple.com>
 

branches/safari-607-branch/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp

@@ -503 +503 @@
         }
         
+        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.
@@ -525 +552 @@
                         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);
-                        }
+                    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 || 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)) {
+                        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;
@@ -552 +590 @@
                             }
                         }
-                    } 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;
+                            }
                         }
                     }