We can't remove code after ForceOSRExit until after FixupPhase
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 04:31:52 +0000 (04:31 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 04:31:52 +0000 (04:31 +0000)
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):

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

JSTests/ChangeLog
JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js [new file with mode: 0644]
JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index ecd3da2..b0c7a68 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         ASSERTION FAILED: regexp->isValid() or ASSERTION FAILED: !isCompilationThread()
diff --git a/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js b/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js
new file mode 100644 (file)
index 0000000..2330488
--- /dev/null
@@ -0,0 +1,16 @@
+function foo(v, a, b) {
+    if (v) {
+        let r = a + b;
+        OSRExit();
+        return r;
+    }
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; ++i) {
+    let r = foo(true, 4, 4);
+    if (r !== 8)
+        throw new Error("Bad!");
+}
+if (foo(true, 2**31 - 1, 1) !== 2**31)
+    throw new Error("Bad!");
diff --git a/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js b/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js
new file mode 100644 (file)
index 0000000..09772de
--- /dev/null
@@ -0,0 +1,16 @@
+function foo(v, a, b) {
+    if (v) {
+        let r = a / b;
+        OSRExit();
+        return r;
+    }
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; ++i) {
+    let r = foo(true, 4, 4);
+    if (r !== 1)
+        throw new Error("Bad!");
+}
+if (foo(true, 1, 4) !== 0.25)
+    throw new Error("Bad!");
index e42ac9c..45829a7 100644 (file)
@@ -1,5 +1,26 @@
 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>
+
         JSScript should have an accessor saying if it's cached or not
         https://bugs.webkit.org/show_bug.cgi?id=195783
 
index 25a5403..ef5029b 100644 (file)
@@ -715,8 +715,6 @@ private:
     {
         VERBOSE_LOG("        appended ", node, " ", Graph::opName(node->op()), "\n");
 
-        m_hasAnyForceOSRExits |= (node->op() == ForceOSRExit);
-
         m_currentBlock->append(node);
         if (clobbersExitState(m_graph, node))
             m_exitOK = false;
@@ -1178,7 +1176,6 @@ private:
 
     const Instruction* m_currentInstruction;
     bool m_hasDebuggerEnabled;
-    bool m_hasAnyForceOSRExits { false };
 };
 
 BasicBlock* ByteCodeParser::allocateTargetableBlock(unsigned bytecodeIndex)
@@ -7287,104 +7284,6 @@ void ByteCodeParser::parse()
     parseCodeBlock();
     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();