Changeset 242989 in webkit

Timestamp:

Mar 14, 2019 9:31:52 PM (5 years ago)

Author:

sbarati@apple.com

Message:

We can't remove code after ForceOSRExit until after FixupPhase
​https://bugs.webkit.org/show_bug.cgi?id=186916
<rdar://problem/41396612>

Reviewed by Yusuke Suzuki.

JSTests:

• stress/movhint-backwards-propagation-must-merge-use-as-value-add.js: Added.

(foo):

• stress/movhint-backwards-propagation-must-merge-use-as-value.js: Added.

(foo):

Source/JavaScriptCore:

There was an optimization in the bytecode parser I added in r232742 that converted blocks
with ForceOSRExit in them to remove all IR after the ForceOSRExit. However,
this is incorrect because it breaks backwards propagation. For example, it
could incorrectly lead us to think it's safe to not check for overflow in
an Add because such Add has no non-int uses. Backwards propagation relies on
having a view over bytecode uses, and this optimization broke that. This patch
rolls out that optimization, as initial perf data shows it may no longer be
needed.

• dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::addToGraph):
(JSC::DFG::ByteCodeParser::parse):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js (added)
• JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-03-14  Saam barati  <sbarati@apple.com>
+
+        We can't remove code after ForceOSRExit until after FixupPhase
+        https://bugs.webkit.org/show_bug.cgi?id=186916
+        <rdar://problem/41396612>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/movhint-backwards-propagation-must-merge-use-as-value-add.js: Added.
+        (foo):
+        * stress/movhint-backwards-propagation-must-merge-use-as-value.js: Added.
+        (foo):
+
 2019-03-13  Michael Saboff  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-03-14  Saam barati  <sbarati@apple.com>
+
+        We can't remove code after ForceOSRExit until after FixupPhase
+        https://bugs.webkit.org/show_bug.cgi?id=186916
+        <rdar://problem/41396612>
+
+        Reviewed by Yusuke Suzuki.
+
+        There was an optimization in the bytecode parser I added in r232742 that converted blocks
+        with ForceOSRExit in them to remove all IR after the ForceOSRExit. However,
+        this is incorrect because it breaks backwards propagation. For example, it
+        could incorrectly lead us to think it's safe to not check for overflow in
+        an Add because such Add has no non-int uses. Backwards propagation relies on
+        having a view over bytecode uses, and this optimization broke that. This patch
+        rolls out that optimization, as initial perf data shows it may no longer be
+        needed.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::addToGraph):
+        (JSC::DFG::ByteCodeParser::parse):
+
 2019-03-14  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -716 +716 @@
         VERBOSE_LOG("        appended ", node, " ", Graph::opName(node->op()), "\n");
 
-        m_hasAnyForceOSRExits |= (node->op() == ForceOSRExit);
-
         m_currentBlock->append(node);
         if (clobbersExitState(m_graph, node))
@@ -1179 +1177 @@
     const Instruction* m_currentInstruction;
     bool m_hasDebuggerEnabled;
-    bool m_hasAnyForceOSRExits { false };
 };
 
@@ -7288 +7285 @@
     linkBlocks(inlineStackEntry.m_unlinkedBlocks, inlineStackEntry.m_blockLinkingTargets);
 
-    if (m_hasAnyForceOSRExits) {
-        BlockSet blocksToIgnore;
-        for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
-            if (block->isOSRTarget && block->bytecodeBegin == m_graph.m_plan.osrEntryBytecodeIndex()) {
-                blocksToIgnore.add(block);
-                break;
-            }
-        }
-
-        {
-            bool isSafeToValidate = false;
-            auto postOrder = m_graph.blocksInPostOrder(isSafeToValidate); // This algorithm doesn't rely on the predecessors list, which is not yet built.
-            bool changed;
-            do {
-                changed = false;
-                for (BasicBlock* block : postOrder) {
-                    for (BasicBlock* successor : block->successors()) {
-                        if (blocksToIgnore.contains(successor)) {
-                            changed |= blocksToIgnore.add(block);
-                            break;
-                        }
-                    }
-                }
-            } while (changed);
-        }
-
-        InsertionSet insertionSet(m_graph);
-        Operands<VariableAccessData*> mapping(OperandsLike, m_graph.block(0)->variablesAtHead);
-
-        for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
-            if (blocksToIgnore.contains(block))
-                continue;
-
-            mapping.fill(nullptr);
-            if (validationEnabled()) {
-                // Verify that it's correct to fill mapping with nullptr.
-                for (unsigned i = 0; i < block->variablesAtHead.size(); ++i) {
-                    Node* node = block->variablesAtHead.at(i);
-                    RELEASE_ASSERT(!node);
-                }
-            }
-
-            for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
-                Node* node = block->at(nodeIndex);
-
-                if (node->hasVariableAccessData(m_graph))
-                    mapping.operand(node->local()) = node->variableAccessData();
-
-                if (node->op() == ForceOSRExit) {
-                    NodeOrigin endOrigin = node->origin.withExitOK(true);
-
-                    if (validationEnabled()) {
-                        // This verifies that we don't need to change any of the successors's predecessor
-                        // list after planting the Unreachable below. At this point in the bytecode
-                        // parser, we haven't linked up the predecessor lists yet.
-                        for (BasicBlock* successor : block->successors())
-                            RELEASE_ASSERT(successor->predecessors.isEmpty());
-                    }
-
-                    block->resize(nodeIndex + 1);
-
-                    insertionSet.insertNode(block->size(), SpecNone, ExitOK, endOrigin);
-
-                    auto insertLivenessPreservingOp = [&] (InlineCallFrame* inlineCallFrame, NodeType op, VirtualRegister operand) {
-                        VariableAccessData* variable = mapping.operand(operand);
-                        if (!variable) {
-                            variable = newVariableAccessData(operand);
-                            mapping.operand(operand) = variable;
-                        }
-
-                        VirtualRegister argument = operand - (inlineCallFrame ? inlineCallFrame->stackOffset : 0);
-                        if (argument.isArgument() && !argument.isHeader()) {
-                            const Vector<ArgumentPosition*>& arguments = m_inlineCallFrameToArgumentPositions.get(inlineCallFrame);
-                            arguments[argument.toArgument()]->addVariable(variable);
-                        } insertionSet.insertNode(block->size(), SpecNone, op, endOrigin, OpInfo(variable));
-                    };
-                    auto addFlushDirect = [&] (InlineCallFrame* inlineCallFrame, VirtualRegister operand) {
-                        insertLivenessPreservingOp(inlineCallFrame, Flush, operand);
-                    };
-                    auto addPhantomLocalDirect = [&] (InlineCallFrame* inlineCallFrame, VirtualRegister operand) {
-                        insertLivenessPreservingOp(inlineCallFrame, PhantomLocal, operand);
-                    };
-                    flushForTerminalImpl(endOrigin.semantic, addFlushDirect, addPhantomLocalDirect);
-
-                    insertionSet.insertNode(block->size(), SpecNone, Unreachable, endOrigin);
-                    insertionSet.execute(block);
-                    break;
-                }
-            }
-        }
-    } else if (validationEnabled()) {
-        // Ensure our bookkeeping for ForceOSRExit nodes is working.
-        for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
-            for (Node* node : *block)
-                RELEASE_ASSERT(node->op() != ForceOSRExit);
-        }
-    }
-    
     m_graph.determineReachability();
     m_graph.killUnreachableBlocks();