Title: [203336] trunk/Source/_javascript_Core
- Revision
- 203336
- Author
- [email protected]
- Date
- 2016-07-17 15:00:42 -0700 (Sun, 17 Jul 2016)
Log Message
DFG CSE is broken for MultiGetByOffset
https://bugs.webkit.org/show_bug.cgi?id=159858
Reviewed by Saam Barati.
This disabled CSE for MultiGetByOffset. I opened bug 159859 for the long-term fix, which
would teach CSE (and other passes also) how to decay a removed MultiGetByOffset to a
CheckStructure. Since we currently just decay MultiGetByOffset to Check, we forget the
structure checks. So, if we CSE a MultiGetByOffset that checks for one set of structures with
a heap access on the same property and base that checks for different structures, then we
will forget some structure checks that we had previously. It's unsound to forget checks in
DFG IR.
This bug mostly manifested as a high-volume crash at Unreachable in FTL, because we'd prove
that the code after the MultiGetByOffset was unreachable due to the structure checks and then
CSE would remove everything but the Unreachable.
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize): Remove the def() for MultiGetByOffset to disable CSE for this node for now.
* tests/stress/cse-multi-get-by-offset-remove-checks.js: Added. This used to fail with FTL eanbled.
(Cons1):
(Cons2):
(Cons3):
(foo):
(bar):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (203335 => 203336)
--- trunk/Source/_javascript_Core/ChangeLog 2016-07-17 20:35:20 UTC (rev 203335)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-07-17 22:00:42 UTC (rev 203336)
@@ -1,3 +1,31 @@
+2016-07-16 Filip Pizlo <[email protected]>
+
+ DFG CSE is broken for MultiGetByOffset
+ https://bugs.webkit.org/show_bug.cgi?id=159858
+
+ Reviewed by Saam Barati.
+
+ This disabled CSE for MultiGetByOffset. I opened bug 159859 for the long-term fix, which
+ would teach CSE (and other passes also) how to decay a removed MultiGetByOffset to a
+ CheckStructure. Since we currently just decay MultiGetByOffset to Check, we forget the
+ structure checks. So, if we CSE a MultiGetByOffset that checks for one set of structures with
+ a heap access on the same property and base that checks for different structures, then we
+ will forget some structure checks that we had previously. It's unsound to forget checks in
+ DFG IR.
+
+ This bug mostly manifested as a high-volume crash at Unreachable in FTL, because we'd prove
+ that the code after the MultiGetByOffset was unreachable due to the structure checks and then
+ CSE would remove everything but the Unreachable.
+
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize): Remove the def() for MultiGetByOffset to disable CSE for this node for now.
+ * tests/stress/cse-multi-get-by-offset-remove-checks.js: Added. This used to fail with FTL enabled.
+ (Cons1):
+ (Cons2):
+ (Cons3):
+ (foo):
+ (bar):
+
2016-07-17 Yusuke Suzuki <[email protected]>
[JSC] Enable test262 module tests
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (203335 => 203336)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2016-07-17 20:35:20 UTC (rev 203335)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2016-07-17 22:00:42 UTC (rev 203336)
@@ -883,7 +883,9 @@
read(JSObject_butterfly);
AbstractHeap heap(NamedProperties, node->multiGetByOffsetData().identifierNumber);
read(heap);
- def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(node));
+ // FIXME: We cannot def() for MultiGetByOffset because CSE is not smart enough to decay it
+ // to a CheckStructure.
+ // https://bugs.webkit.org/show_bug.cgi?id=159859
return;
}
Added: trunk/Source/_javascript_Core/tests/stress/cse-multi-get-by-offset-remove-checks.js (0 => 203336)
--- trunk/Source/_javascript_Core/tests/stress/cse-multi-get-by-offset-remove-checks.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/cse-multi-get-by-offset-remove-checks.js 2016-07-17 22:00:42 UTC (rev 203336)
@@ -0,0 +1,55 @@
+function Cons1()
+{
+ this.e = 1;
+ this.f = 2;
+}
+
+Cons1.prototype.g = 1;
+
+function Cons2()
+{
+ this.f = 1;
+ this.h = 2;
+}
+
+Cons2.prototype.g = 2;
+
+function Cons3()
+{
+ this.d = 1;
+ this.e = 2;
+ this.f = 3;
+}
+
+Cons3.prototype = Cons2.prototype;
+
+function foo(o, p, q)
+{
+ var x = 0, y = 0;
+ if (p)
+ x = o.f;
+ if (q)
+ y = o.f;
+ return x + y;
+}
+
+for (var i = 0; i < 10000; ++i) {
+ foo(new Cons1(), true, false);
+ foo(new Cons2(), false, true);
+ foo(new Cons3(), false, true);
+}
+
+function bar(o, p)
+{
+ return foo(o, true, p);
+}
+
+noInline(bar);
+
+for (var i = 0; i < 100000; ++i)
+ bar(new Cons1(), false);
+
+var result = bar(new Cons1(), true);
+if (result != 4)
+ throw "Error: bad result: " + result;
+
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
