DFG is too aggressive eliding overflow checks for additions involving large constants

https://bugs.webkit.org/show_bug.cgi?id=105239

Reviewed by Gavin Barraclough.

Source/JavaScriptCore:

If we elide overflow checks on an addition (or subtraction) involving a larger-than-2^32 immediate,
then make sure that the non-constant child of the addition knows that he's got to do an overflow
check, by flowing the UsedAsNumber property at him.

* dfg/DFGGraph.h:
(JSC::DFG::Graph::addSpeculationMode):
(Graph):
(JSC::DFG::Graph::addShouldSpeculateInteger):
(JSC::DFG::Graph::addImmediateShouldSpeculateInteger):
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):

LayoutTests:

* fast/js/dfg-int-overflow-large-constants-in-a-line-expected.txt: Added.
* fast/js/dfg-int-overflow-large-constants-in-a-line.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-int-overflow-large-constants-in-a-line.js: Added.
(foo):

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

2 files changed, 37 insertions, 11 deletions

diff --git a/Source/JavaScriptCore/dfg/DFGGraph.h b/Source/JavaScriptCore/dfg/DFGGraph.h
index d91d37394..b39845968 100644
--- a/Source/JavaScriptCore/dfg/DFGGraph.h
+++ b/Source/JavaScriptCore/dfg/DFGGraph.h
@@ -72,6 +72,12 @@ struct PutToBaseOperationData {
     unsigned putToBaseOperationIndex;
 };
 
+enum AddSpeculationMode {
+    DontSpeculateInteger,
+    SpeculateIntegerButAlwaysWatchOverflow,
+    SpeculateInteger
+};
+    
 
 //
 // === Graph ===
@@ -209,7 +215,7 @@ public:
         return speculationFromValue(node.valueOfJSConstant(m_codeBlock));
     }
     
-    bool addShouldSpeculateInteger(Node& add)
+    AddSpeculationMode addSpeculationMode(Node& add)
     {
         ASSERT(add.op() == ValueAdd || add.op() == ArithAdd || add.op() == ArithSub);
         
@@ -221,7 +227,12 @@ public:
         if (right.hasConstant())
             return addImmediateShouldSpeculateInteger(add, left, right);
         
-        return Node::shouldSpeculateIntegerExpectingDefined(left, right) && add.canSpeculateInteger();
+        return (Node::shouldSpeculateIntegerExpectingDefined(left, right) && add.canSpeculateInteger()) ? SpeculateInteger : DontSpeculateInteger;
+    }
+    
+    bool addShouldSpeculateInteger(Node& add)
+    {
+        return addSpeculationMode(add) != DontSpeculateInteger;
     }
     
     bool mulShouldSpeculateInteger(Node& mul)
@@ -699,26 +710,26 @@ private:
     
     void handleSuccessor(Vector<BlockIndex, 16>& worklist, BlockIndex blockIndex, BlockIndex successorIndex);
     
-    bool addImmediateShouldSpeculateInteger(Node& add, Node& variable, Node& immediate)
+    AddSpeculationMode addImmediateShouldSpeculateInteger(Node& add, Node& variable, Node& immediate)
     {
         ASSERT(immediate.hasConstant());
         
         JSValue immediateValue = immediate.valueOfJSConstant(m_codeBlock);
         if (!immediateValue.isNumber())
-            return false;
+            return DontSpeculateInteger;
         
         if (!variable.shouldSpeculateIntegerExpectingDefined())
-            return false;
+            return DontSpeculateInteger;
         
         if (immediateValue.isInt32())
-            return add.canSpeculateInteger();
+            return add.canSpeculateInteger() ? SpeculateInteger : DontSpeculateInteger;
         
         double doubleImmediate = immediateValue.asDouble();
         const double twoToThe48 = 281474976710656.0;
         if (doubleImmediate < -twoToThe48 || doubleImmediate > twoToThe48)
-            return false;
+            return DontSpeculateInteger;
         
-        return nodeCanTruncateInteger(add.arithNodeFlags());
+        return nodeCanTruncateInteger(add.arithNodeFlags()) ? SpeculateIntegerButAlwaysWatchOverflow : DontSpeculateInteger;
     }
     
     bool mulImmediateShouldSpeculateInteger(Node& mul, Node& variable, Node& immediate)
diff --git a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
index 67270101e..5b6c28ff7 100644
--- a/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
@@ -285,9 +285,11 @@ private:
             SpeculatedType left = m_graph[node.child1()].prediction();
             SpeculatedType right = m_graph[node.child2()].prediction();
             
+            AddSpeculationMode mode = DontSpeculateInteger;
+            
             if (left && right) {
                 if (isNumberSpeculationExpectingDefined(left) && isNumberSpeculationExpectingDefined(right)) {
-                    if (m_graph.addShouldSpeculateInteger(node))
+                    if ((mode = m_graph.addSpeculationMode(node)) != DontSpeculateInteger)
                         changed |= mergePrediction(SpecInt32);
                     else
                         changed |= mergePrediction(speculatedDoubleTypeForPredictions(left, right));
@@ -303,6 +305,9 @@ private:
             if (m_graph[node.child1()].hasNumberResult() || m_graph[node.child2()].hasNumberResult())
                 flags &= ~NodeUsedAsOther;
             
+            if (mode != SpeculateInteger)
+                flags |= NodeUsedAsNumber;
+            
             changed |= m_graph[node.child1()].mergeFlags(flags);
             changed |= m_graph[node.child2()].mergeFlags(flags);
             break;
@@ -312,8 +317,10 @@ private:
             SpeculatedType left = m_graph[node.child1()].prediction();
             SpeculatedType right = m_graph[node.child2()].prediction();
             
+            AddSpeculationMode mode = DontSpeculateInteger;
+            
             if (left && right) {
-                if (m_graph.addShouldSpeculateInteger(node))
+                if ((mode = m_graph.addSpeculationMode(node)) != DontSpeculateInteger)
                     changed |= mergePrediction(SpecInt32);
                 else
                     changed |= mergePrediction(speculatedDoubleTypeForPredictions(left, right));
@@ -323,6 +330,9 @@ private:
                 flags &= ~NodeNeedsNegZero;
             flags &= ~NodeUsedAsOther;
             
+            if (mode != SpeculateInteger)
+                flags |= NodeUsedAsNumber;
+            
             changed |= m_graph[node.child1()].mergeFlags(flags);
             changed |= m_graph[node.child2()].mergeFlags(flags);
             break;
@@ -332,8 +342,10 @@ private:
             SpeculatedType left = m_graph[node.child1()].prediction();
             SpeculatedType right = m_graph[node.child2()].prediction();
             
+            AddSpeculationMode mode = DontSpeculateInteger;
+            
             if (left && right) {
-                if (m_graph.addShouldSpeculateInteger(node))
+                if ((mode = m_graph.addSpeculationMode(node)) != DontSpeculateInteger)
                     changed |= mergePrediction(SpecInt32);
                 else
                     changed |= mergePrediction(speculatedDoubleTypeForPredictions(left, right));
@@ -343,6 +355,9 @@ private:
                 flags &= ~NodeNeedsNegZero;
             flags &= ~NodeUsedAsOther;
             
+            if (mode != SpeculateInteger)
+                flags |= NodeUsedAsNumber;
+            
             changed |= m_graph[node.child1()].mergeFlags(flags);
             changed |= m_graph[node.child2()].mergeFlags(flags);
             break;