Title: [129266] trunk
Revision
129266
Author
[email protected]
Date
2012-09-21 15:59:37 -0700 (Fri, 21 Sep 2012)

Log Message

DFG CSE assumes that a holy PutByVal does not interfere with GetArrayLength, when it clearly does
https://bugs.webkit.org/show_bug.cgi?id=97373

Reviewed by Mark Hahnenberg.

Source/_javascript_Core: 

* dfg/DFGCSEPhase.cpp:
(JSC::DFG::CSEPhase::pureCSE):
(JSC::DFG::CSEPhase::getArrayLengthElimination):
(JSC::DFG::CSEPhase::putStructureStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE):
* dfg/DFGGraph.h:
(Graph):

LayoutTests: 

* fast/js/dfg-holy-put-by-val-interferes-with-get-array-length-expected.txt: Added.
* fast/js/dfg-holy-put-by-val-interferes-with-get-array-length.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-holy-put-by-val-interferes-with-get-array-length.js: Added.
(foo):

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (129265 => 129266)


--- trunk/LayoutTests/ChangeLog	2012-09-21 22:35:50 UTC (rev 129265)
+++ trunk/LayoutTests/ChangeLog	2012-09-21 22:59:37 UTC (rev 129266)
@@ -1,3 +1,16 @@
+2012-09-21  Filip Pizlo  <[email protected]>
+
+        DFG CSE assumes that a holy PutByVal does not interfere with GetArrayLength, when it clearly does
+        https://bugs.webkit.org/show_bug.cgi?id=97373
+
+        Reviewed by Mark Hahnenberg.
+
+        * fast/js/dfg-holy-put-by-val-interferes-with-get-array-length-expected.txt: Added.
+        * fast/js/dfg-holy-put-by-val-interferes-with-get-array-length.html: Added.
+        * fast/js/jsc-test-list:
+        * fast/js/script-tests/dfg-holy-put-by-val-interferes-with-get-array-length.js: Added.
+        (foo):
+
 2012-09-21  Roger Fong  <[email protected]>
 
         Unreviewed. Skipping http/tests/security/sandboxed-iframe-origin-add.html.

Added: trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length-expected.txt (0 => 129266)


--- trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length-expected.txt	2012-09-21 22:59:37 UTC (rev 129266)
@@ -0,0 +1,109 @@
+Tests that the DFG's interference analysis knows that a holy PutByVal interferes with a GetArrayLength.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS foo([75]) is [1,2]
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length.html (0 => 129266)


--- trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length.html	                        (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-holy-put-by-val-interferes-with-get-array-length.html	2012-09-21 22:59:37 UTC (rev 129266)
@@ -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 (129265 => 129266)


--- trunk/LayoutTests/fast/js/jsc-test-list	2012-09-21 22:35:50 UTC (rev 129265)
+++ trunk/LayoutTests/fast/js/jsc-test-list	2012-09-21 22:59:37 UTC (rev 129266)
@@ -99,6 +99,7 @@
 fast/js/dfg-get-by-val-getter-cse
 fast/js/dfg-getter-throw
 fast/js/dfg-getter
+fast/js/dfg-holy-put-by-val-interferes-with-get-array-length
 fast/js/dfg-inline-arguments-become-double
 fast/js/dfg-inline-arguments-become-int32
 fast/js/dfg-inline-arguments-int32

Added: trunk/LayoutTests/fast/js/script-tests/dfg-holy-put-by-val-interferes-with-get-array-length.js (0 => 129266)


--- trunk/LayoutTests/fast/js/script-tests/dfg-holy-put-by-val-interferes-with-get-array-length.js	                        (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-holy-put-by-val-interferes-with-get-array-length.js	2012-09-21 22:59:37 UTC (rev 129266)
@@ -0,0 +1,14 @@
+description(
+"Tests that the DFG's interference analysis knows that a holy PutByVal interferes with a GetArrayLength."
+);
+
+function foo(array) {
+    var x = array.length;
+    array[1] = 42;
+    return [x, array.length];
+}
+
+for (var i = 0; i < 100; ++i)
+    shouldBe("foo([75])", "[1,2]");
+
+

Modified: trunk/Source/_javascript_Core/ChangeLog (129265 => 129266)


--- trunk/Source/_javascript_Core/ChangeLog	2012-09-21 22:35:50 UTC (rev 129265)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-09-21 22:59:37 UTC (rev 129266)
@@ -1,3 +1,18 @@
+2012-09-21  Filip Pizlo  <[email protected]>
+
+        DFG CSE assumes that a holy PutByVal does not interfere with GetArrayLength, when it clearly does
+        https://bugs.webkit.org/show_bug.cgi?id=97373
+
+        Reviewed by Mark Hahnenberg.
+
+        * dfg/DFGCSEPhase.cpp:
+        (JSC::DFG::CSEPhase::pureCSE):
+        (JSC::DFG::CSEPhase::getArrayLengthElimination):
+        (JSC::DFG::CSEPhase::putStructureStoreElimination):
+        (JSC::DFG::CSEPhase::performNodeCSE):
+        * dfg/DFGGraph.h:
+        (Graph):
+
 2012-09-21  Chris Rogers  <[email protected]>
 
         Add Web Audio support for deprecated/legacy APIs

Modified: trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp (129265 => 129266)


--- trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp	2012-09-21 22:35:50 UTC (rev 129265)
+++ trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp	2012-09-21 22:59:37 UTC (rev 129266)
@@ -96,6 +96,9 @@
                 break;
 
             Node& otherNode = m_graph[index];
+            if (!otherNode.shouldGenerate())
+                continue;
+            
             if (node.op() != otherNode.op())
                 continue;
             
@@ -157,38 +160,34 @@
         return NoNode;
     }
     
-    NodeIndex impureCSE(Node& node)
+    NodeIndex getArrayLengthElimination(NodeIndex array)
     {
-        NodeIndex child1 = canonicalize(node.child1());
-        NodeIndex child2 = canonicalize(node.child2());
-        NodeIndex child3 = canonicalize(node.child3());
-        
         for (unsigned i = m_indexInBlock; i--;) {
             NodeIndex index = m_currentBlock->at(i);
-            if (index == child1 || index == child2 || index == child3)
-                break;
-
-            Node& otherNode = m_graph[index];
-            if (node.op() == otherNode.op()
-                && node.arithNodeFlags() == otherNode.arithNodeFlags()) {
-                NodeIndex otherChild = canonicalize(otherNode.child1());
-                if (otherChild == NoNode)
+            Node& node = m_graph[index];
+            if (!node.shouldGenerate())
+                continue;
+            switch (node.op()) {
+            case GetArrayLength:
+                if (node.child1() == array)
                     return index;
-                if (otherChild == child1) {
-                    otherChild = canonicalize(otherNode.child2());
-                    if (otherChild == NoNode)
-                        return index;
-                    if (otherChild == child2) {
-                        otherChild = canonicalize(otherNode.child3());
-                        if (otherChild == NoNode)
-                            return index;
-                        if (otherChild == child3)
-                            return index;
-                    }
+                break;
+                
+            case PutByVal:
+                if (!m_graph.byValIsPure(node))
+                    return NoNode;
+                switch (node.arrayMode()) {
+                case ARRAY_STORAGE_TO_HOLE_MODES:
+                    return NoNode;
+                default:
+                    break;
                 }
-            }
-            if (m_graph.clobbersWorld(index))
                 break;
+                
+            default:
+                if (m_graph.clobbersWorld(index))
+                    return NoNode;
+            }
         }
         return NoNode;
     }
@@ -437,7 +436,7 @@
                 break;
             Node& node = m_graph[index];
             if (!node.shouldGenerate())
-                break;
+                continue;
             switch (node.op()) {
             case CheckStructure:
             case ForwardCheckStructure:
@@ -1078,7 +1077,7 @@
             break;
             
         case GetArrayLength:
-            setReplacement(impureCSE(node));
+            setReplacement(getArrayLengthElimination(node.child1().index()));
             break;
             
         case GetScopeChain:

Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (129265 => 129266)


--- trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-09-21 22:35:50 UTC (rev 129265)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h	2012-09-21 22:59:37 UTC (rev 129266)
@@ -469,6 +469,12 @@
         return isNumberSpeculation(left) && isNumberSpeculation(right);
     }
     
+    // Note that a 'true' return does not actually mean that the ByVal access clobbers nothing.
+    // It really means that it will not clobber the entire world. It's still up to you to
+    // carefully consider things like:
+    // - PutByVal definitely changes the array it stores to, and may even change its length.
+    // - PutByOffset definitely changes the object it stores to.
+    // - and so on.
     bool byValIsPure(Node& node)
     {
         switch (node.arrayMode()) {
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to