- Revision
- 242989
- Author
- [email protected]
- Date
- 2019-03-14 21:31:52 -0700 (Thu, 14 Mar 2019)
Log 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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (242988 => 242989)
--- trunk/JSTests/ChangeLog 2019-03-15 03:31:40 UTC (rev 242988)
+++ trunk/JSTests/ChangeLog 2019-03-15 04:31:52 UTC (rev 242989)
@@ -1,3 +1,16 @@
+2019-03-14 Saam barati <[email protected]>
+
+ 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 <[email protected]>
ASSERTION FAILED: regexp->isValid() or ASSERTION FAILED: !isCompilationThread()
Added: trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js (0 => 242989)
--- trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js (rev 0)
+++ trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value-add.js 2019-03-15 04:31:52 UTC (rev 242989)
@@ -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!");
Added: trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js (0 => 242989)
--- trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js (rev 0)
+++ trunk/JSTests/stress/movhint-backwards-propagation-must-merge-use-as-value.js 2019-03-15 04:31:52 UTC (rev 242989)
@@ -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!");
Modified: trunk/Source/_javascript_Core/ChangeLog (242988 => 242989)
--- trunk/Source/_javascript_Core/ChangeLog 2019-03-15 03:31:40 UTC (rev 242988)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-03-15 04:31:52 UTC (rev 242989)
@@ -1,5 +1,26 @@
2019-03-14 Saam barati <[email protected]>
+ 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 <[email protected]>
+
JSScript should have an accessor saying if it's cached or not
https://bugs.webkit.org/show_bug.cgi?id=195783
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (242988 => 242989)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-03-15 03:31:40 UTC (rev 242988)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-03-15 04:31:52 UTC (rev 242989)
@@ -715,8 +715,6 @@
{
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 @@
const Instruction* m_currentInstruction;
bool m_hasDebuggerEnabled;
- bool m_hasAnyForceOSRExits { false };
};
BasicBlock* ByteCodeParser::allocateTargetableBlock(unsigned bytecodeIndex)
@@ -7287,104 +7284,6 @@
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();
