Log Message
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. JSTests: * stress/regress-170990.js: Added. Source/_javascript_Core: 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):
Modified Paths
- trunk/JSTests/ChangeLog
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp
- trunk/Source/_javascript_Core/dfg/DFGClobberize.h
- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp
- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (215747 => 215748)
--- trunk/JSTests/ChangeLog 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/JSTests/ChangeLog 2017-04-25 19:04:47 UTC (rev 215748)
@@ -1,3 +1,13 @@
+2017-04-25 Mark Lam <mark....@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....@gmail.com>
WebAssembly: exporting a property with a name that's a number doesn't work
Added: trunk/JSTests/stress/regress-170990.js (0 => 215748)
--- trunk/JSTests/stress/regress-170990.js (rev 0)
+++ trunk/JSTests/stress/regress-170990.js 2017-04-25 19:04:47 UTC (rev 215748)
@@ -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);
Modified: trunk/Source/_javascript_Core/ChangeLog (215747 => 215748)
--- trunk/Source/_javascript_Core/ChangeLog 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-04-25 19:04:47 UTC (rev 215748)
@@ -1,5 +1,25 @@
2017-04-25 Mark Lam <mark....@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....@apple.com>
+
Make DFG SpeculatedType dumps easier to read.
https://bugs.webkit.org/show_bug.cgi?id=171280
Modified: trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp (215747 => 215748)
--- trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2017-04-25 19:04:47 UTC (rev 215748)
@@ -460,6 +460,7 @@
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,7 +468,7 @@
if (!mode.isInBounds())
break;
heap = HeapLocation(
- IndexedPropertyLoc, IndexedInt32Properties, base, index);
+ indexedPropertyLoc, IndexedInt32Properties, base, index);
break;
case Array::Double:
@@ -474,7 +475,7 @@
if (!mode.isInBounds())
break;
heap = HeapLocation(
- IndexedPropertyLoc, IndexedDoubleProperties, base, index);
+ indexedPropertyLoc, IndexedDoubleProperties, base, index);
break;
case Array::Contiguous:
@@ -481,7 +482,7 @@
if (!mode.isInBounds())
break;
heap = HeapLocation(
- IndexedPropertyLoc, IndexedContiguousProperties, base, index);
+ indexedPropertyLoc, IndexedContiguousProperties, base, index);
break;
case Array::Int8Array:
@@ -496,7 +497,7 @@
if (!mode.isInBounds())
break;
heap = HeapLocation(
- IndexedPropertyLoc, TypedArrayProperties, base, index);
+ indexedPropertyLoc, TypedArrayProperties, base, index);
break;
default:
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (215747 => 215748)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2017-04-25 19:04:47 UTC (rev 215748)
@@ -693,6 +693,7 @@
case GetByVal: {
ArrayMode mode = node->arrayMode();
+ LocationKind indexedPropertyLoc = indexedPropertyLocForResultType(node->result());
switch (mode.type()) {
case Array::SelectUsingPredictions:
case Array::Unprofiled:
@@ -723,12 +724,12 @@
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:
@@ -735,7 +736,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
JSValue* data = ""
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 @@
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));
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp (215747 => 215748)
--- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp 2017-04-25 19:04:47 UTC (rev 215748)
@@ -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 @@
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;
Modified: trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h (215747 => 215748)
--- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h 2017-04-25 18:42:33 UTC (rev 215747)
+++ trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h 2017-04-25 19:04:47 UTC (rev 215748)
@@ -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 @@
GetterLoc,
GlobalVariableLoc,
HasIndexedPropertyLoc,
- IndexedPropertyLoc,
+ IndexedPropertyDoubleLoc,
+ IndexedPropertyInt52Loc,
+ IndexedPropertyJSLoc,
IndexedPropertyStorageLoc,
InstanceOfLoc,
InvalidationPointLoc,
@@ -138,6 +140,29 @@
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 {
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
