Occasional failure in v8-v6/v8-raytrace.js.ftl-eager
authorbasile_clement@apple.com <basile_clement@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Aug 2015 05:00:57 +0000 (05:00 +0000)
committerbasile_clement@apple.com <basile_clement@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Aug 2015 05:00:57 +0000 (05:00 +0000)
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.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js [new file with mode: 0644]

index f623d0b..0395a08 100644 (file)
@@ -1,3 +1,26 @@
+2015-08-14  Basile Clement  <basile_clement@apple.com>
+
+        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  <fpizlo@apple.com>
 
         Use WTF::Lock and WTF::Condition instead of WTF::Mutex, WTF::ThreadCondition, std::mutex, and std::condition_variable
index fd88910..3e21c4a 100644 (file)
@@ -921,14 +921,67 @@ private:
             }
             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 = node->multiGetByOffsetData();
+                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());
index c61a751..04f234b 100644 (file)
@@ -5515,6 +5515,11 @@ private:
         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,
diff --git a/Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js b/Source/JavaScriptCore/tests/stress/sink-multigetbyoffset.js
new file mode 100644 (file)
index 0000000..29c4dbb
--- /dev/null
@@ -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);
+}