[webkit-changes] [137980] trunk

Mon, 17 Dec 2012 20:33:25 -0800

Title: [137980] trunk
Revision
137980
Author
[email protected]
Date
2012-12-17 20:35:10 -0800 (Mon, 17 Dec 2012)

Log Message

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

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

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (137979 => 137980)


--- trunk/LayoutTests/ChangeLog	2012-12-18 04:33:40 UTC (rev 137979)
+++ trunk/LayoutTests/ChangeLog	2012-12-18 04:35:10 UTC (rev 137980)
@@ -1,3 +1,16 @@
+2012-12-17  Filip Pizlo  <[email protected]>
+
+        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.
+
+        * 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):
+
 2012-12-17  Shinya Kawanaka  <[email protected]>
 
         Web Inspector: need to visually distinguish UA shadow roots

Added: trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line-expected.txt (0 => 137980)


--- trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line-expected.txt	2012-12-18 04:35:10 UTC (rev 137980)
@@ -0,0 +1,209 @@
+Tests that our optimization to elide overflow checks understands that if we keep adding huge numbers, we could end up creating a number that is not precisely representable using doubles.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS foo(2147483647) is 2147483552
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line.html (0 => 137980)


--- trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line.html	                        (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-int-overflow-large-constants-in-a-line.html	2012-12-18 04:35:10 UTC (rev 137980)
@@ -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 (137979 => 137980)


--- trunk/LayoutTests/fast/js/jsc-test-list	2012-12-18 04:33:40 UTC (rev 137979)
+++ trunk/LayoutTests/fast/js/jsc-test-list	2012-12-18 04:35:10 UTC (rev 137980)
@@ -134,6 +134,7 @@
 fast/js/dfg-inline-unused-this
 fast/js/dfg-inlining-reg-alloc
 fast/js/dfg-int-overflow-in-loop
+fast/js/dfg-int-overflow-large-constants-in-a-line
 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-large-constants-in-a-line.js (0 => 137980)


--- trunk/LayoutTests/fast/js/script-tests/dfg-int-overflow-large-constants-in-a-line.js	                        (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-int-overflow-large-constants-in-a-line.js	2012-12-18 04:35:10 UTC (rev 137980)
@@ -0,0 +1,53 @@
+description(
+"Tests that our optimization to elide overflow checks understands that if we keep adding huge numbers, we could end up creating a number that is not precisely representable using doubles."
+);
+
+function foo(a) {
+    var x = a;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    x += 281474976710655;
+    x += 281474976710654;
+    x += 281474976710653;
+    x += 281474976710652;
+    return x | 0
+}
+
+for (var i = 0; i < 200; ++i)
+    shouldBe("foo(2147483647)", "2147483552");
+
+

Modified: trunk/Source/_javascript_Core/ChangeLog (137979 => 137980)


--- trunk/Source/_javascript_Core/ChangeLog	2012-12-18 04:33:40 UTC (rev 137979)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-12-18 04:35:10 UTC (rev 137980)
@@ -1,3 +1,22 @@
+2012-12-17  Filip Pizlo  <[email protected]>
+
+        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.
+
+        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):
+
 2012-12-17  Michael Saboff  <[email protected]>
 
         DFG: Refactor DFGCorrectableJumpPoint to reduce size of OSRExit data

Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (137979 => 137980)


--- trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-12-18 04:33:40 UTC (rev 137979)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-12-18 04:35:10 UTC (rev 137980)
@@ -72,6 +72,12 @@
     unsigned putToBaseOperationIndex;
 };
 
+enum AddSpeculationMode {
+    DontSpeculateInteger,
+    SpeculateIntegerButAlwaysWatchOverflow,
+    SpeculateInteger
+};
+    
 
 //
 // === Graph ===
@@ -209,7 +215,7 @@
         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,9 +227,14 @@
         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)
     {
         ASSERT(mul.op() == ArithMul);
@@ -700,26 +711,26 @@
     
     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)

Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (137979 => 137980)


--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp	2012-12-18 04:33:40 UTC (rev 137979)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp	2012-12-18 04:35:10 UTC (rev 137980)
@@ -291,9 +291,11 @@
             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));
@@ -309,6 +311,9 @@
             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;
@@ -318,8 +323,10 @@
             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));
@@ -329,6 +336,9 @@
                 flags &= ~NodeNeedsNegZero;
             flags &= ~NodeUsedAsOther;
             
+            if (mode != SpeculateInteger)
+                flags |= NodeUsedAsNumber;
+            
             changed |= m_graph[node.child1()].mergeFlags(flags);
             changed |= m_graph[node.child2()].mergeFlags(flags);
             break;
@@ -338,8 +348,10 @@
             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));
@@ -349,6 +361,9 @@
                 flags &= ~NodeNeedsNegZero;
             flags &= ~NodeUsedAsOther;
             
+            if (mode != SpeculateInteger)
+                flags |= NodeUsedAsNumber;
+            
             changed |= m_graph[node.child1()].mergeFlags(flags);
             changed |= m_graph[node.child2()].mergeFlags(flags);
             break;

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to