Local CSE wrongly CSEs array accesses with different result types.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Apr 2017 19:04:47 +0000 (19:04 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Apr 2017 19:04:47 +0000 (19:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170990
<rdar://problem/31705945>

Reviewed by Saam Barati.

JSTests:

* stress/regress-170990.js: Added.

Source/JavaScriptCore:

The fix is to use different LocationKind enums for the different type of array
result types.  This makes the HeapLocation values different based on the result
types, and allows CSE to discern between them.

* dfg/DFGCSEPhase.cpp:
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGHeapLocation.cpp:
(WTF::printInternal):
* dfg/DFGHeapLocation.h:
(JSC::DFG::indexedPropertyLocForResultType):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@215748 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/regress-170990.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGHeapLocation.cpp
Source/JavaScriptCore/dfg/DFGHeapLocation.h

index 1e64980..83f4c01 100644 (file)
@@ -1,3 +1,13 @@
+2017-04-25  Mark Lam  <mark.lam@apple.com>
+
+        Local CSE wrongly CSEs array accesses with different result types.
+        https://bugs.webkit.org/show_bug.cgi?id=170990
+        <rdar://problem/31705945>
+
+        Reviewed by Saam Barati.
+
+        * stress/regress-170990.js: Added.
+
 2017-04-25  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         WebAssembly: exporting a property with a name that's a number doesn't work
diff --git a/JSTests/stress/regress-170990.js b/JSTests/stress/regress-170990.js
new file mode 100644 (file)
index 0000000..c7c457b
--- /dev/null
@@ -0,0 +1,15 @@
+function getter(arr, operand, resultArr) { 
+     resultArr[0] = arr[operand];
+}
+
+function test(resultArr, arr, getter) {
+    getter(arr, 0, resultArr);
+    getter(arr, 1, resultArr);
+    resultArr[0] == arr[1];
+}
+noInline(run);
+
+var arr = new Uint32Array([0x80000000,1]); 
+var resultArr = [];
+for (var i = 0; i < 10000; i++)
+    test(resultArr, arr, getter);
index 1c02ff2..0939ef3 100644 (file)
@@ -1,5 +1,25 @@
 2017-04-25  Mark Lam  <mark.lam@apple.com>
 
+        Local CSE wrongly CSEs array accesses with different result types.
+        https://bugs.webkit.org/show_bug.cgi?id=170990
+        <rdar://problem/31705945>
+
+        Reviewed by Saam Barati.
+
+        The fix is to use different LocationKind enums for the different type of array
+        result types.  This makes the HeapLocation values different based on the result
+        types, and allows CSE to discern between them.
+
+        * dfg/DFGCSEPhase.cpp:
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGHeapLocation.cpp:
+        (WTF::printInternal):
+        * dfg/DFGHeapLocation.h:
+        (JSC::DFG::indexedPropertyLocForResultType):
+
+2017-04-25  Mark Lam  <mark.lam@apple.com>
+
         Make DFG SpeculatedType dumps easier to read.
         https://bugs.webkit.org/show_bug.cgi?id=171280
 
index 699049b..5968974 100644 (file)
@@ -460,6 +460,7 @@ private:
                         
                         Node* base = m_graph.varArgChild(m_node, 0).node();
                         Node* index = m_graph.varArgChild(m_node, 1).node();
+                        LocationKind indexedPropertyLoc = indexedPropertyLocForResultType(m_node->result());
                         
                         ArrayMode mode = m_node->arrayMode();
                         switch (mode.type()) {
@@ -467,21 +468,21 @@ private:
                             if (!mode.isInBounds())
                                 break;
                             heap = HeapLocation(
-                                IndexedPropertyLoc, IndexedInt32Properties, base, index);
+                                indexedPropertyLoc, IndexedInt32Properties, base, index);
                             break;
                             
                         case Array::Double:
                             if (!mode.isInBounds())
                                 break;
                             heap = HeapLocation(
-                                IndexedPropertyLoc, IndexedDoubleProperties, base, index);
+                                indexedPropertyLoc, IndexedDoubleProperties, base, index);
                             break;
                             
                         case Array::Contiguous:
                             if (!mode.isInBounds())
                                 break;
                             heap = HeapLocation(
-                                IndexedPropertyLoc, IndexedContiguousProperties, base, index);
+                                indexedPropertyLoc, IndexedContiguousProperties, base, index);
                             break;
                             
                         case Array::Int8Array:
@@ -496,7 +497,7 @@ private:
                             if (!mode.isInBounds())
                                 break;
                             heap = HeapLocation(
-                                IndexedPropertyLoc, TypedArrayProperties, base, index);
+                                indexedPropertyLoc, TypedArrayProperties, base, index);
                             break;
                             
                         default:
index e7576cd..2af9564 100644 (file)
@@ -693,6 +693,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         
     case GetByVal: {
         ArrayMode mode = node->arrayMode();
+        LocationKind indexedPropertyLoc = indexedPropertyLocForResultType(node->result());
         switch (mode.type()) {
         case Array::SelectUsingPredictions:
         case Array::Unprofiled:
@@ -723,19 +724,19 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             
         case Array::DirectArguments:
             read(DirectArgumentsProperties);
-            def(HeapLocation(IndexedPropertyLoc, DirectArgumentsProperties, node->child1(), node->child2()), LazyNode(node));
+            def(HeapLocation(indexedPropertyLoc, DirectArgumentsProperties, node->child1(), node->child2()), LazyNode(node));
             return;
             
         case Array::ScopedArguments:
             read(ScopeProperties);
-            def(HeapLocation(IndexedPropertyLoc, ScopeProperties, node->child1(), node->child2()), LazyNode(node));
+            def(HeapLocation(indexedPropertyLoc, ScopeProperties, node->child1(), node->child2()), LazyNode(node));
             return;
             
         case Array::Int32:
             if (mode.isInBounds()) {
                 read(Butterfly_publicLength);
                 read(IndexedInt32Properties);
-                def(HeapLocation(IndexedPropertyLoc, IndexedInt32Properties, node->child1(), node->child2()), LazyNode(node));
+                def(HeapLocation(indexedPropertyLoc, IndexedInt32Properties, node->child1(), node->child2()), LazyNode(node));
                 return;
             }
             read(World);
@@ -746,7 +747,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             if (mode.isInBounds()) {
                 read(Butterfly_publicLength);
                 read(IndexedDoubleProperties);
-                def(HeapLocation(IndexedPropertyLoc, IndexedDoubleProperties, node->child1(), node->child2()), LazyNode(node));
+                def(HeapLocation(indexedPropertyLoc, IndexedDoubleProperties, node->child1(), node->child2()), LazyNode(node));
                 return;
             }
             read(World);
@@ -757,7 +758,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             if (mode.isInBounds()) {
                 read(Butterfly_publicLength);
                 read(IndexedContiguousProperties);
-                def(HeapLocation(IndexedPropertyLoc, IndexedContiguousProperties, node->child1(), node->child2()), LazyNode(node));
+                def(HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, node->child1(), node->child2()), LazyNode(node));
                 return;
             }
             read(World);
@@ -790,7 +791,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         case Array::Float64Array:
             read(TypedArrayProperties);
             read(MiscFields);
-            def(HeapLocation(IndexedPropertyLoc, TypedArrayProperties, node->child1(), node->child2()), LazyNode(node));
+            def(HeapLocation(indexedPropertyLoc, TypedArrayProperties, node->child1(), node->child2()), LazyNode(node));
             return;
         // We should not get an AnyTypedArray in a GetByVal as AnyTypedArray is only created from intrinsics, which
         // are only added from Inline Caching a GetById.
@@ -817,6 +818,8 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         Node* base = graph.varArgChild(node, 0).node();
         Node* index = graph.varArgChild(node, 1).node();
         Node* value = graph.varArgChild(node, 2).node();
+        LocationKind indexedPropertyLoc = indexedPropertyLocForResultType(node->result());
+
         switch (mode.modeForPut().type()) {
         case Array::SelectUsingPredictions:
         case Array::SelectUsingArguments:
@@ -848,7 +851,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             write(IndexedInt32Properties);
             if (node->arrayMode().mayStoreToHole())
                 write(Butterfly_publicLength);
-            def(HeapLocation(IndexedPropertyLoc, IndexedInt32Properties, base, index), LazyNode(value));
+            def(HeapLocation(indexedPropertyLoc, IndexedInt32Properties, base, index), LazyNode(value));
             return;
             
         case Array::Double:
@@ -863,7 +866,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             write(IndexedDoubleProperties);
             if (node->arrayMode().mayStoreToHole())
                 write(Butterfly_publicLength);
-            def(HeapLocation(IndexedPropertyLoc, IndexedDoubleProperties, base, index), LazyNode(value));
+            def(HeapLocation(indexedPropertyLoc, IndexedDoubleProperties, base, index), LazyNode(value));
             return;
             
         case Array::Contiguous:
@@ -878,7 +881,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             write(IndexedContiguousProperties);
             if (node->arrayMode().mayStoreToHole())
                 write(Butterfly_publicLength);
-            def(HeapLocation(IndexedPropertyLoc, IndexedContiguousProperties, base, index), LazyNode(value));
+            def(HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, base, index), LazyNode(value));
             return;
             
         case Array::ArrayStorage:
@@ -1243,17 +1246,21 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             return;
 
         AbstractHeap heap;
+        LocationKind indexedPropertyLoc;
         switch (node->indexingType()) {
         case ALL_DOUBLE_INDEXING_TYPES:
             heap = IndexedDoubleProperties;
+            indexedPropertyLoc = IndexedPropertyDoubleLoc;
             break;
 
         case ALL_INT32_INDEXING_TYPES:
             heap = IndexedInt32Properties;
+            indexedPropertyLoc = IndexedPropertyJSLoc;
             break;
 
         case ALL_CONTIGUOUS_INDEXING_TYPES:
             heap = IndexedContiguousProperties;
+            indexedPropertyLoc = IndexedPropertyJSLoc;
             break;
 
         default:
@@ -1263,7 +1270,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         if (numElements < graph.m_uint32ValuesInUse.size()) {
             for (unsigned operandIdx = 0; operandIdx < numElements; ++operandIdx) {
                 Edge use = graph.m_varArgChildren[node->firstChild() + operandIdx];
-                def(HeapLocation(IndexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(operandIdx)))),
+                def(HeapLocation(indexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(operandIdx)))),
                     LazyNode(use.node()));
             }
         } else {
@@ -1272,7 +1279,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
                     continue;
                 Edge use = graph.m_varArgChildren[node->firstChild() + operandIdx];
                 // operandIdx comes from graph.m_uint32ValuesInUse and thus is guaranteed to be already frozen
-                def(HeapLocation(IndexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(operandIdx)))),
+                def(HeapLocation(indexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(operandIdx)))),
                     LazyNode(use.node()));
             }
         }
@@ -1288,19 +1295,23 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
             LazyNode(graph.freeze(jsNumber(numElements))));
 
         AbstractHeap heap;
+        LocationKind indexedPropertyLoc;
         NodeType op = JSConstant;
         switch (node->indexingType()) {
         case ALL_DOUBLE_INDEXING_TYPES:
             heap = IndexedDoubleProperties;
+            indexedPropertyLoc = IndexedPropertyDoubleLoc;
             op = DoubleConstant;
             break;
 
         case ALL_INT32_INDEXING_TYPES:
             heap = IndexedInt32Properties;
+            indexedPropertyLoc = IndexedPropertyJSLoc;
             break;
 
         case ALL_CONTIGUOUS_INDEXING_TYPES:
             heap = IndexedContiguousProperties;
+            indexedPropertyLoc = IndexedPropertyJSLoc;
             break;
 
         default:
@@ -1310,7 +1321,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         JSValue* data = graph.m_codeBlock->constantBuffer(node->startConstant());
         if (numElements < graph.m_uint32ValuesInUse.size()) {
             for (unsigned index = 0; index < numElements; ++index) {
-                def(HeapLocation(IndexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(index)))),
+                def(HeapLocation(indexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(index)))),
                     LazyNode(graph.freeze(data[index]), op));
             }
         } else {
@@ -1321,7 +1332,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
                 possibleIndices.append(index);
             }
             for (uint32_t index : possibleIndices) {
-                def(HeapLocation(IndexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(index)))),
+                def(HeapLocation(indexedPropertyLoc, heap, node, LazyNode(graph.freeze(jsNumber(index)))),
                     LazyNode(graph.freeze(data[index]), op));
             }
         }
index 9b2292d..790c341 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -115,11 +115,19 @@ void printInternal(PrintStream& out, LocationKind kind)
     case HasIndexedPropertyLoc:
         out.print("HasIndexedPorpertyLoc");
         return;
-        
-    case IndexedPropertyLoc:
-        out.print("IndexedPorpertyLoc");
+
+    case IndexedPropertyDoubleLoc:
+        out.print("IndexedPropertyDoubleLoc");
         return;
-        
+
+    case IndexedPropertyInt52Loc:
+        out.print("IndexedPropertyInt52Loc");
+        return;
+
+    case IndexedPropertyJSLoc:
+        out.print("IndexedPropertyJSLoc");
+        return;
+
     case IndexedPropertyStorageLoc:
         out.print("IndexedPropertyStorageLoc");
         return;
index 6357be1..3ea8920 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -45,7 +45,9 @@ enum LocationKind {
     GetterLoc,
     GlobalVariableLoc,
     HasIndexedPropertyLoc,
-    IndexedPropertyLoc,
+    IndexedPropertyDoubleLoc,
+    IndexedPropertyInt52Loc,
+    IndexedPropertyJSLoc,
     IndexedPropertyStorageLoc,
     InstanceOfLoc,
     InvalidationPointLoc,
@@ -138,6 +140,29 @@ struct HeapLocationHash {
     static const bool safeToCompareToEmptyOrDeleted = true;
 };
 
+LocationKind indexedPropertyLocForResultType(NodeFlags);
+
+inline LocationKind indexedPropertyLocForResultType(NodeFlags canonicalResultRepresentation)
+{
+    if (!canonicalResultRepresentation)
+        return IndexedPropertyJSLoc;
+
+    ASSERT((canonicalResultRepresentation & NodeResultMask) == canonicalResultRepresentation);
+    switch (canonicalResultRepresentation) {
+    case NodeResultDouble:
+        return IndexedPropertyDoubleLoc;
+    case NodeResultInt52:
+        return IndexedPropertyInt52Loc;
+    case NodeResultJS:
+        return IndexedPropertyJSLoc;
+    case NodeResultStorage:
+        RELEASE_ASSERT_NOT_REACHED();
+    default:
+        break;
+    }
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
 } } // namespace JSC::DFG
 
 namespace WTF {