- Revision
- 262425
- Author
- sbar...@apple.com
- Date
- 2020-06-02 09:55:15 -0700 (Tue, 02 Jun 2020)
Log Message
MultiDeleteByOffset should not always def
https://bugs.webkit.org/show_bug.cgi?id=212621
<rdar://problem/63824182>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/multi-del-by-offset-doesnt-always-def-osr-entry.js: Added.
(foo):
* stress/multi-del-by-offset-doesnt-always-def.js: Added.
(foo):
(let.p.set undefined):
Source/_javascript_Core:
Clobberize used to claim that MultiDeleteByOffset always defd a value.
That's an incorrect modeling of MultiDeleteByOffset though, since it might
have delete misses in its variant list. This would lead us to incorrectly
CSE when we shouldn't. This patch fixes this by saying MultiDeleteByOffset
only defs when all its cases write out a value (are hits).
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGNode.cpp:
(JSC::DFG::MultiDeleteByOffsetData::allVariantsStoreEmpty const):
* dfg/DFGNode.h:
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileMultiDeleteByOffset):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (262424 => 262425)
--- trunk/JSTests/ChangeLog 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/JSTests/ChangeLog 2020-06-02 16:55:15 UTC (rev 262425)
@@ -1,3 +1,17 @@
+2020-06-02 Saam Barati <sbar...@apple.com>
+
+ MultiDeleteByOffset should not always def
+ https://bugs.webkit.org/show_bug.cgi?id=212621
+ <rdar://problem/63824182>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/multi-del-by-offset-doesnt-always-def-osr-entry.js: Added.
+ (foo):
+ * stress/multi-del-by-offset-doesnt-always-def.js: Added.
+ (foo):
+ (let.p.set undefined):
+
2020-06-01 Yusuke Suzuki <ysuz...@apple.com>
[JSC] JSBigInt::rightTrim can miss |this| pointer and leads to incorrect GC collection
Added: trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def-osr-entry.js (0 => 262425)
--- trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def-osr-entry.js (rev 0)
+++ trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def-osr-entry.js 2020-06-02 16:55:15 UTC (rev 262425)
@@ -0,0 +1,13 @@
+let p = { get: undefined, set: undefined, };
+
+function foo() {
+ let o = {};
+ for (let i = 0; i < 100; i++) {
+ for (let j = 0; j < 5; j++)
+ delete o.x;
+ o.x;
+ Object.defineProperty(o, 'x', p);
+ }
+}
+
+foo()
Added: trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def.js (0 => 262425)
--- trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def.js (rev 0)
+++ trunk/JSTests/stress/multi-del-by-offset-doesnt-always-def.js 2020-06-02 16:55:15 UTC (rev 262425)
@@ -0,0 +1,14 @@
+let p = { get: undefined, set: undefined, };
+let o1 = {};
+Object.defineProperty(o1, 'x', p);
+
+function foo(o) {
+ if (!(delete o.x))
+ o.x;
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; ++i) {
+ foo(o1);
+ foo({x : 42});
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (262424 => 262425)
--- trunk/Source/_javascript_Core/ChangeLog 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-06-02 16:55:15 UTC (rev 262425)
@@ -1,3 +1,25 @@
+2020-06-02 Saam Barati <sbar...@apple.com>
+
+ MultiDeleteByOffset should not always def
+ https://bugs.webkit.org/show_bug.cgi?id=212621
+ <rdar://problem/63824182>
+
+ Reviewed by Yusuke Suzuki.
+
+ Clobberize used to claim that MultiDeleteByOffset always defd a value.
+ That's an incorrect modeling of MultiDeleteByOffset though, since it might
+ have delete misses in its variant list. This would lead us to incorrectly
+ CSE when we shouldn't. This patch fixes this by saying MultiDeleteByOffset
+ only defs when all its cases write out a value (are hits).
+
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGNode.cpp:
+ (JSC::DFG::MultiDeleteByOffsetData::allVariantsStoreEmpty const):
+ * dfg/DFGNode.h:
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileMultiDeleteByOffset):
+
2020-06-02 Rob Buis <rb...@igalia.com>
Make generated C++ code use modern C++
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (262424 => 262425)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2020-06-02 16:55:15 UTC (rev 262425)
@@ -1338,7 +1338,8 @@
// alias analysis.
write(NamedProperties);
}
- def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(graph.freezeStrong(JSValue())));
+ if (node->multiDeleteByOffsetData().allVariantsStoreEmpty())
+ def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(graph.freezeStrong(JSValue())));
return;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.cpp (262424 => 262425)
--- trunk/Source/_javascript_Core/dfg/DFGNode.cpp 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.cpp 2020-06-02 16:55:15 UTC (rev 262425)
@@ -66,6 +66,15 @@
return false;
}
+bool MultiDeleteByOffsetData::allVariantsStoreEmpty() const
+{
+ for (unsigned i = variants.size(); i--;) {
+ if (!variants[i].newStructure())
+ return false;
+ }
+ return true;
+}
+
void BranchTarget::dump(PrintStream& out) const
{
if (!block)
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (262424 => 262425)
--- trunk/Source/_javascript_Core/dfg/DFGNode.h 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h 2020-06-02 16:55:15 UTC (rev 262425)
@@ -98,6 +98,7 @@
Vector<DeleteByIdVariant, 2> variants;
bool writesStructures() const;
+ bool allVariantsStoreEmpty() const;
};
struct MatchStructureVariant {
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (262424 => 262425)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-06-02 16:10:54 UTC (rev 262424)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2020-06-02 16:55:15 UTC (rev 262425)
@@ -8471,22 +8471,20 @@
m_out.appendTo(blocks[block], block + 1 < blocks.size() ? blocks[block + 1] : exit);
- if (variant.newStructure()) {
- LValue storage;
+ LValue storage;
- if (isInlineOffset(variant.offset()))
- storage = base;
- else
- storage = m_out.loadPtr(base, m_heaps.JSObject_butterfly);
+ if (isInlineOffset(variant.offset()))
+ storage = base;
+ else
+ storage = m_out.loadPtr(base, m_heaps.JSObject_butterfly);
- storeProperty(m_out.int64Zero, storage, data.identifierNumber, variant.offset());
+ storeProperty(m_out.int64Zero, storage, data.identifierNumber, variant.offset());
- ASSERT(variant.oldStructure()->indexingType() == variant.newStructure()->indexingType());
- ASSERT(variant.oldStructure()->typeInfo().inlineTypeFlags() == variant.newStructure()->typeInfo().inlineTypeFlags());
- ASSERT(variant.oldStructure()->typeInfo().type() == variant.newStructure()->typeInfo().type());
- m_out.store32(
- weakStructureID(m_graph.registerStructure(variant.newStructure())), base, m_heaps.JSCell_structureID);
- }
+ ASSERT(variant.oldStructure()->indexingType() == variant.newStructure()->indexingType());
+ ASSERT(variant.oldStructure()->typeInfo().inlineTypeFlags() == variant.newStructure()->typeInfo().inlineTypeFlags());
+ ASSERT(variant.oldStructure()->typeInfo().type() == variant.newStructure()->typeInfo().type());
+ m_out.store32(
+ weakStructureID(m_graph.registerStructure(variant.newStructure())), base, m_heaps.JSCell_structureID);
results.append(m_out.anchor(variant.result() ? m_out.booleanTrue : m_out.booleanFalse));
m_out.jump(continuation);
