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/JavaScriptCore:

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):

Change-Id: I9df2d6d17ba404802456f4e2da313e47f0f4f62e
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@137963 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Reviewed-by: Jocelyn Turcotte <jocelyn.turcotte@digia.com>

2 files changed, 8 insertions, 2 deletions

diff --git a/Source/JavaScriptCore/dfg/DFGNodeFlags.h b/Source/JavaScriptCore/dfg/DFGNodeFlags.h
index 5e41bfc6b..463451c39 100644
--- a/Source/JavaScriptCore/dfg/DFGNodeFlags.h
+++ b/Source/JavaScriptCore/dfg/DFGNodeFlags.h
@@ -54,7 +54,7 @@ namespace JSC { namespace DFG {
                                 
 #define NodeBackPropMask         0x3C00
 #define NodeUseBottom             0x000
-#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)
diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index 5b6c28ff7..4226fcc6a 100644
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -211,7 +211,13 @@ private:
         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;
         }