Title: [188507] trunk/Source/_javascript_Core
- Revision
- 188507
- Author
- [email protected]
- Date
- 2015-08-14 22:00:57 -0700 (Fri, 14 Aug 2015)
Log Message
Occasional failure in v8-v6/v8-raytrace.js.ftl-eager
https://bugs.webkit.org/show_bug.cgi?id=147165
Reviewed by Saam Barati.
The object allocation sinking phase was not properly checking that a
MultiGetByOffset was safe to lower before lowering it.
This makes it so that we only lower MultiGetByOffset if it only loads
from direct properties of the object, and considers it as an escape in
any other case (e.g. a load from the prototype).
It also ensure proper conversion of MultiGetByOffset into
CheckStructureImmediate when needed.
* dfg/DFGObjectAllocationSinkingPhase.cpp:
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::checkStructure):
We were not compiling properly CheckStructure and
CheckStructureImmediate nodes with an empty StructureSet.
* tests/stress/sink-multigetbyoffset.js: Regression test.
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (188506 => 188507)
--- trunk/Source/_javascript_Core/ChangeLog 2015-08-15 03:47:04 UTC (rev 188506)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-08-15 05:00:57 UTC (rev 188507)
@@ -1,3 +1,26 @@
+2015-08-14 Basile Clement <[email protected]>
+
+ Occasional failure in v8-v6/v8-raytrace.js.ftl-eager
+ https://bugs.webkit.org/show_bug.cgi?id=147165
+
+ Reviewed by Saam Barati.
+
+ The object allocation sinking phase was not properly checking that a
+ MultiGetByOffset was safe to lower before lowering it.
+ This makes it so that we only lower MultiGetByOffset if it only loads
+ from direct properties of the object, and considers it as an escape in
+ any other case (e.g. a load from the prototype).
+
+ It also ensure proper conversion of MultiGetByOffset into
+ CheckStructureImmediate when needed.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ * ftl/FTLLowerDFGToLLVM.cpp:
+ (JSC::FTL::DFG::LowerDFGToLLVM::checkStructure):
+ We were not compiling properly CheckStructure and
+ CheckStructureImmediate nodes with an empty StructureSet.
+ * tests/stress/sink-multigetbyoffset.js: Regression test.
+
2015-08-14 Filip Pizlo <[email protected]>
Use WTF::Lock and WTF::Condition instead of WTF::Mutex, WTF::ThreadCondition, std::mutex, and std::condition_variable
Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (188506 => 188507)
--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2015-08-15 03:47:04 UTC (rev 188506)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2015-08-15 05:00:57 UTC (rev 188507)
@@ -921,14 +921,67 @@
}
break;
- case MultiGetByOffset:
- target = m_heap.onlyLocalAllocation(node->child1().node());
- if (target && target->isObjectAllocation()) {
- unsigned identifierNumber = node->multiGetByOffsetData().identifierNumber;
- exactRead = PromotedLocationDescriptor(NamedPropertyPLoc, identifierNumber);
+ case MultiGetByOffset: {
+ Allocation* allocation = m_heap.onlyLocalAllocation(node->child1().node());
+ if (allocation && allocation->isObjectAllocation()) {
+ MultiGetByOffsetData& data = ""
+ StructureSet validStructures;
+ bool hasInvalidStructures = false;
+ for (const auto& multiGetByOffsetCase : data.cases) {
+ if (!allocation->structures().overlaps(multiGetByOffsetCase.set()))
+ continue;
+
+ switch (multiGetByOffsetCase.method().kind()) {
+ case GetByOffsetMethod::LoadFromPrototype: // We need to escape those
+ case GetByOffsetMethod::Constant: // We don't really have a way of expressing this
+ hasInvalidStructures = true;
+ break;
+
+ case GetByOffsetMethod::Load: // We're good
+ validStructures.merge(multiGetByOffsetCase.set());
+ break;
+
+ default:
+ RELEASE_ASSERT_NOT_REACHED();
+ }
+ }
+ if (hasInvalidStructures) {
+ m_heap.escape(node->child1().node());
+ break;
+ }
+ unsigned identifierNumber = data.identifierNumber;
+ PromotedHeapLocation location(NamedPropertyPLoc, allocation->identifier(), identifierNumber);
+ if (Node* value = heapResolve(location)) {
+ if (allocation->structures().isSubsetOf(validStructures))
+ node->replaceWith(value);
+ else {
+ Node* structure = heapResolve(PromotedHeapLocation(allocation->identifier(), StructurePLoc));
+ ASSERT(structure);
+ allocation->filterStructures(validStructures);
+ node->convertToCheckStructure(m_graph.addStructureSet(allocation->structures()));
+ node->convertToCheckStructureImmediate(structure);
+ node->setReplacement(value);
+ }
+ } else if (!allocation->structures().isSubsetOf(validStructures)) {
+ // Even though we don't need the result here, we still need
+ // to make the call to tell our caller that we could need
+ // the StructurePLoc.
+ // The reason for this is that when we decide not to sink a
+ // node, we will still lower any read to its fields before
+ // it escapes (which are usually reads across a function
+ // call that DFGClobberize can't handle) - but we only do
+ // this for PromotedHeapLocations that we have seen read
+ // during the analysis!
+ heapResolve(PromotedHeapLocation(allocation->identifier(), StructurePLoc));
+ allocation->filterStructures(validStructures);
+ }
+ Node* identifier = allocation->get(location.descriptor());
+ if (identifier)
+ m_heap.newPointer(node, identifier);
} else
m_heap.escape(node->child1().node());
break;
+ }
case PutByOffset:
target = m_heap.onlyLocalAllocation(node->child2().node());
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp (188506 => 188507)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp 2015-08-15 03:47:04 UTC (rev 188506)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp 2015-08-15 05:00:57 UTC (rev 188507)
@@ -5515,6 +5515,11 @@
LValue structureDiscriminant, const FormattedValue& formattedValue, ExitKind exitKind,
const StructureSet& set, const Functor& weakStructureDiscriminant)
{
+ if (set.isEmpty()) {
+ terminate(exitKind);
+ return;
+ }
+
if (set.size() == 1) {
speculate(
exitKind, formattedValue, 0,
Added: trunk/Source/_javascript_Core/tests/stress/sink-multigetbyoffset.js (0 => 188507)
--- trunk/Source/_javascript_Core/tests/stress/sink-multigetbyoffset.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/sink-multigetbyoffset.js 2015-08-15 05:00:57 UTC (rev 188507)
@@ -0,0 +1,27 @@
+// Regression test for https://bugs.webkit.org/show_bug.cgi?id=147165
+
+function Foo() { }
+Foo.prototype.f = 42;
+
+function get(o, p) {
+ if (p)
+ return o.f;
+ return 42;
+}
+
+for (var i = 0; i < 100000; ++i) {
+ get({ f: 42 }, i % 2);
+ get({ o: 10, f: 42 }, i % 2);
+}
+
+function foo() {
+ var o = new Foo();
+ return get(o, isFinalTier());
+}
+noInline(foo);
+
+for (var i = 0; i < 1000000; ++i) {
+ var result = foo();
+ if (result !== 42)
+ throw new Error("Result should be 42 but was " + result);
+}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
