Log Message
Constant folding an access to an uncaptured variable that is captured later in the same basic block shouldn't lead to assertion failures https://bugs.webkit.org/show_bug.cgi?id=107750 <rdar://problem/12387265>
Source/_javascript_Core:
Reviewed by Mark Hahnenberg.
The point of this assertion was that if there is no variable capturing going on, then there should only be one GetLocal
for the variable anywhere in the basic block. But if there is some capturing, then we'll have an unbounded number of
GetLocals. The assertion was too imprecise for the latter case. I want to keep this assertion, so I introduced a
checker that verifies this precisely: if there are any captured accesses to the variable anywhere at or after the
GetLocal we are eliminating, then we allow redundant GetLocals.
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
(ConstantFoldingPhase):
(JSC::DFG::ConstantFoldingPhase::isCapturedAtOrAfter):
LayoutTests:
Reviewed by Mark Hahnenberg.
* fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured-expected.txt: Added.
* fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-constant-fold-uncaptured-variable-that-is-later-captured.js: Added.
(bar):
(baz):
(foo):
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/LayoutTests/fast/js/jsc-test-list
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (140607 => 140608)
--- trunk/LayoutTests/ChangeLog 2013-01-23 23:30:38 UTC (rev 140607)
+++ trunk/LayoutTests/ChangeLog 2013-01-24 00:09:59 UTC (rev 140608)
@@ -1,3 +1,19 @@
+2013-01-23 Filip Pizlo <[email protected]>
+
+ Constant folding an access to an uncaptured variable that is captured later in the same basic block shouldn't lead to assertion failures
+ https://bugs.webkit.org/show_bug.cgi?id=107750
+ <rdar://problem/12387265>
+
+ Reviewed by Mark Hahnenberg.
+
+ * fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured-expected.txt: Added.
+ * fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html: Added.
+ * fast/js/jsc-test-list:
+ * fast/js/script-tests/dfg-constant-fold-uncaptured-variable-that-is-later-captured.js: Added.
+ (bar):
+ (baz):
+ (foo):
+
2013-01-23 Hans Muller <[email protected]>
[CSS Exclusions] Add support for computing first included interval position for polygons
Added: trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured-expected.txt (0 => 140608)
--- trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured-expected.txt 2013-01-24 00:09:59 UTC (rev 140608)
@@ -0,0 +1,109 @@
+Tests that constant folding an access to an uncaptured variable that is captured later in the same basic block doesn't lead to assertion failures.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS foo(true, 5)[0] is 462
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html (0 => 140608)
--- trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured.html 2013-01-24 00:09:59 UTC (rev 140608)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Modified: trunk/LayoutTests/fast/js/jsc-test-list (140607 => 140608)
--- trunk/LayoutTests/fast/js/jsc-test-list 2013-01-23 23:30:38 UTC (rev 140607)
+++ trunk/LayoutTests/fast/js/jsc-test-list 2013-01-24 00:09:59 UTC (rev 140608)
@@ -96,6 +96,7 @@
fast/js/dfg-check-two-structures
fast/js/dfg-check-structure-elimination-for-non-cell
fast/js/dfg-constant-fold-first-local-read-after-block-merge
+fast/js/dfg-constant-fold-uncaptured-variable-that-is-later-captured
fast/js/dfg-convert-this-dom-window
fast/js/dfg-create-inlined-arguments-in-closure-inline
fast/js/dfg-cse-cfa-discrepancy
Added: trunk/LayoutTests/fast/js/script-tests/dfg-constant-fold-uncaptured-variable-that-is-later-captured.js (0 => 140608)
--- trunk/LayoutTests/fast/js/script-tests/dfg-constant-fold-uncaptured-variable-that-is-later-captured.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-constant-fold-uncaptured-variable-that-is-later-captured.js 2013-01-24 00:09:59 UTC (rev 140608)
@@ -0,0 +1,29 @@
+description(
+"Tests that constant folding an access to an uncaptured variable that is captured later in the same basic block doesn't lead to assertion failures."
+);
+
+var thingy = 456;
+
+function bar() {
+ return thingy;
+}
+
+function baz(a) {
+ if (a) // Here we have an access to r2. The bug was concerned with our assertions thinking that this access was invalid.
+ return arguments; // Force r2 (see below) to get captured.
+}
+
+function foo(p, a) {
+ // The temporary variable corresponding to the 'bar' callee coming out of the ternary _expression_ will be allocated by
+ // the bytecompiler to some virtual register, say r2. This _expression_ is engineered so that (1) the virtual register
+ // chosen for the callee here is the same as the one that will be chosen for the first non-this argument below,
+ // (2) that the callee ends up being constant but requires CFA to prove it, and (3) that we actually load that constant
+ // using GetLocal (which happens because of the CheckFunction to check the callee).
+ var x = (a + 1) + (p ? bar : bar)();
+ // The temporary variable corresponding to the first non-this argument to baz will be allocated to the same virtual
+ // register (i.e. r2).
+ return baz(x);
+}
+
+for (var i = 0; i < 100; ++i)
+ shouldBe("foo(true, 5)[0]", "462");
Modified: trunk/Source/_javascript_Core/ChangeLog (140607 => 140608)
--- trunk/Source/_javascript_Core/ChangeLog 2013-01-23 23:30:38 UTC (rev 140607)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-01-24 00:09:59 UTC (rev 140608)
@@ -1,3 +1,22 @@
+2013-01-23 Filip Pizlo <[email protected]>
+
+ Constant folding an access to an uncaptured variable that is captured later in the same basic block shouldn't lead to assertion failures
+ https://bugs.webkit.org/show_bug.cgi?id=107750
+ <rdar://problem/12387265>
+
+ Reviewed by Mark Hahnenberg.
+
+ The point of this assertion was that if there is no variable capturing going on, then there should only be one GetLocal
+ for the variable anywhere in the basic block. But if there is some capturing, then we'll have an unbounded number of
+ GetLocals. The assertion was too imprecise for the latter case. I want to keep this assertion, so I introduced a
+ checker that verifies this precisely: if there are any captured accesses to the variable anywhere at or after the
+ GetLocal we are eliminating, then we allow redundant GetLocals.
+
+ * dfg/DFGConstantFoldingPhase.cpp:
+ (JSC::DFG::ConstantFoldingPhase::foldConstants):
+ (ConstantFoldingPhase):
+ (JSC::DFG::ConstantFoldingPhase::isCapturedAtOrAfter):
+
2013-01-23 Oliver Hunt <[email protected]>
Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC
Modified: trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (140607 => 140608)
--- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2013-01-23 23:30:38 UTC (rev 140607)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2013-01-24 00:09:59 UTC (rev 140608)
@@ -373,9 +373,10 @@
if (tailNodeIndex == nodeIndex)
block->variablesAtTail.operand(node.local()) = previousLocalAccess;
else {
- ASSERT(m_graph[tailNodeIndex].op() == Flush
+ ASSERT(
+ m_graph[tailNodeIndex].op() == Flush
|| m_graph[tailNodeIndex].op() == SetLocal
- || node.variableAccessData()->isCaptured());
+ || isCapturedAtOrAfter(block, indexInBlock, node.local()));
}
}
@@ -394,6 +395,22 @@
return changed;
}
+#if !ASSERT_DISABLED
+ bool isCapturedAtOrAfter(BasicBlock* block, unsigned indexInBlock, int operand)
+ {
+ for (; indexInBlock < block->size(); ++indexInBlock) {
+ Node& node = m_graph[block->at(indexInBlock)];
+ if (!node.hasLocal())
+ continue;
+ if (node.local() != operand)
+ continue;
+ if (node.variableAccessData()->isCaptured())
+ return true;
+ }
+ return false;
+ }
+#endif // !ASSERT_DISABLED
+
void addStructureTransitionCheck(CodeOrigin codeOrigin, unsigned indexInBlock, JSCell* cell)
{
NodeIndex weakConstantIndex = m_insertionSet.insertNode(
_______________________________________________ webkit-changes mailing list [email protected] http://lists.webkit.org/mailman/listinfo/webkit-changes
