- Revision
- 137963
- Author
- [email protected]
- Date
- 2012-12-17 17:35:13 -0800 (Mon, 17 Dec 2012)
Log Message
DFG is too aggressive with eliding overflow checks in loops
https://bugs.webkit.org/show_bug.cgi?id=105226
Reviewed by Mark Hahnenberg and Oliver Hunt.
Source/_javascript_Core:
If we see a variable's live range cross basic block boundaries, conservatively assume that it may
be part of a data-flow back-edge, and as a result, we may have entirely integer operations that
could lead to the creation of an integer that is out of range of 2^52 (the significand of a double
float). This does not seem to regress any of the benchmarks we care about, and it fixes the bug.
In future we may want to actually look at whether or not there was a data-flow back-edge instead
of being super conservative about it. But we have no evidence, yet, that this would help us on
real code.
* dfg/DFGNodeFlags.h:
(DFG):
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
LayoutTests:
* fast/js/dfg-int-overflow-in-loop-expected.txt: Added.
* fast/js/dfg-int-overflow-in-loop.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-int-overflow-in-loop.js: Added.
(foo):
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (137962 => 137963)
--- trunk/LayoutTests/ChangeLog 2012-12-18 01:21:55 UTC (rev 137962)
+++ trunk/LayoutTests/ChangeLog 2012-12-18 01:35:13 UTC (rev 137963)
@@ -1,3 +1,16 @@
+2012-12-17 Filip Pizlo <[email protected]>
+
+ DFG is too aggressive with eliding overflow checks in loops
+ https://bugs.webkit.org/show_bug.cgi?id=105226
+
+ Reviewed by Mark Hahnenberg and Oliver Hunt.
+
+ * fast/js/dfg-int-overflow-in-loop-expected.txt: Added.
+ * fast/js/dfg-int-overflow-in-loop.html: Added.
+ * fast/js/jsc-test-list:
+ * fast/js/script-tests/dfg-int-overflow-in-loop.js: Added.
+ (foo):
+
2012-12-17 Chris Fleizach <[email protected]>
Seamless iframe should not announce a new browsing context
Added: trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop-expected.txt (0 => 137963)
--- trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop-expected.txt 2012-12-18 01:35:13 UTC (rev 137963)
@@ -0,0 +1,10 @@
+Tests that overflowing an integer in a loop and then only using it in an integer context produces a result that complies with double arithmetic.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS foo(0) is -4094336
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop.html (0 => 137963)
--- trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop.html (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-int-overflow-in-loop.html 2012-12-18 01:35:13 UTC (rev 137963)
@@ -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 (137962 => 137963)
--- trunk/LayoutTests/fast/js/jsc-test-list 2012-12-18 01:21:55 UTC (rev 137962)
+++ trunk/LayoutTests/fast/js/jsc-test-list 2012-12-18 01:35:13 UTC (rev 137963)
@@ -133,6 +133,7 @@
fast/js/dfg-inline-unused-this-method-check
fast/js/dfg-inline-unused-this
fast/js/dfg-inlining-reg-alloc
+fast/js/dfg-int-overflow-in-loop
fast/js/dfg-int16array
fast/js/dfg-int32-to-double-on-known-number
fast/js/dfg-int32array-overflow-values
Added: trunk/LayoutTests/fast/js/script-tests/dfg-int-overflow-in-loop.js (0 => 137963)
--- trunk/LayoutTests/fast/js/script-tests/dfg-int-overflow-in-loop.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-int-overflow-in-loop.js 2012-12-18 01:35:13 UTC (rev 137963)
@@ -0,0 +1,17 @@
+description(
+"Tests that overflowing an integer in a loop and then only using it in an integer context produces a result that complies with double arithmetic."
+);
+
+function foo(a) {
+ var x = a;
+ // Make sure that this is the loop where we do OSR entry.
+ for (var i = 0; i < 100000; ++i)
+ x += 1;
+ // Now trigger overflow that is so severe that the floating point result would be different than the bigint result.
+ for (var i = 0; i < 160097152; ++i)
+ x += 2147483647;
+ return x | 0;
+}
+
+shouldBe("foo(0)", "-4094336");
+
Modified: trunk/Source/_javascript_Core/ChangeLog (137962 => 137963)
--- trunk/Source/_javascript_Core/ChangeLog 2012-12-18 01:21:55 UTC (rev 137962)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-12-18 01:35:13 UTC (rev 137963)
@@ -1,3 +1,24 @@
+2012-12-17 Filip Pizlo <[email protected]>
+
+ DFG is too aggressive with eliding overflow checks in loops
+ https://bugs.webkit.org/show_bug.cgi?id=105226
+
+ Reviewed by Mark Hahnenberg and Oliver Hunt.
+
+ If we see a variable's live range cross basic block boundaries, conservatively assume that it may
+ be part of a data-flow back-edge, and as a result, we may have entirely integer operations that
+ could lead to the creation of an integer that is out of range of 2^52 (the significand of a double
+ float). This does not seem to regress any of the benchmarks we care about, and it fixes the bug.
+
+ In future we may want to actually look at whether or not there was a data-flow back-edge instead
+ of being super conservative about it. But we have no evidence, yet, that this would help us on
+ real code.
+
+ * dfg/DFGNodeFlags.h:
+ (DFG):
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ (JSC::DFG::PredictionPropagationPhase::propagate):
+
2012-12-17 Mark Hahnenberg <[email protected]>
Butterfly::growArrayRight shouldn't be called on null Butterfly objects
Modified: trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h (137962 => 137963)
--- trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h 2012-12-18 01:21:55 UTC (rev 137962)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h 2012-12-18 01:35:13 UTC (rev 137963)
@@ -54,7 +54,7 @@
#define NodeBackPropMask 0x7C00
#define NodeUseBottom 0x0000
-#define NodeUsedAsNumber 0x400 // The result of this computation may be used in a context that observes fractional results.
+#define NodeUsedAsNumber 0x400 // The result of this computation may be used in a context that observes fractional, or bigger-than-int32, results.
#define NodeNeedsNegZero 0x800 // The result of this computation may be used in a context that observes -0.
#define NodeUsedAsOther 0x1000 // The result of this computation may be used in a context that distinguishes between NaN and other things (like undefined).
#define NodeUsedAsValue (NodeUsedAsNumber | NodeNeedsNegZero | NodeUsedAsOther)
Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (137962 => 137963)
--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2012-12-18 01:21:55 UTC (rev 137962)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2012-12-18 01:35:13 UTC (rev 137963)
@@ -211,7 +211,13 @@
case SetLocal: {
VariableAccessData* variableAccessData = node.variableAccessData();
changed |= variableAccessData->predict(m_graph[node.child1()].prediction());
- changed |= m_graph[node.child1()].mergeFlags(variableAccessData->flags());
+
+ // Assume conservatively that a SetLocal implies that the value may flow through a loop,
+ // and so we would have overflow leading to the program "observing" numbers even if all
+ // users of the value are doing toInt32. It might be worthwhile to revisit this at some
+ // point and actually check if the data flow involves loops, but right now I don't think
+ // we have evidence that this would be beneficial for benchmarks.
+ changed |= m_graph[node.child1()].mergeFlags(variableAccessData->flags() | NodeUsedAsNumber);
break;
}
