Object allocation sinking phase doesn't properly handle control flow …

…when emitting a PutHint of a materialized object into a PromotedHeapLocation of a still sunken object

https://bugs.webkit.org/show_bug.cgi?id=168140
<rdar://problem/30205880>

Reviewed by Filip Pizlo.

JSTests:

* stress/allocation-sinking-puthint-control-flow.js: Added.
(e):
(bar):
(let.y):
(else.let.y):
(baz):
(foo):
(catch):

Source/JavaScriptCore:

This patch fixes a bug in allocation sinking phase where
we don't properly handle control flow when materializing
an object and also PutHinting that materialization into
a still sunken object. We were performing the PutHint
for the materialization at the point of materialization,
however, we may have materialized along both edges
of a control flow diamond, in which case, we need to
also PutHint at the join point. Consider this program:

```
bb#0:
b: PhantomActivation()
a: PhantomNewFunction()
c: PutHint(@A, @b, ActivationLoc)
Branch(#1, #2)

bb#1:
d: MaterializeActivation()
e: PutHint(@A, @d, ActivationLoc)
f: Upsilon(@d, ^p)
Jump(#3)

bb#2:
g: MaterializeActivation()
h: PutHint(@A, @g, ActivationLoc)
i: Upsilon(@d, ^p)
Jump(#3)

bb#3:
p: Phi()
// What is PromotedHeapLocation(@A, ActivationLoc) here?
// What would we do if we exited?
```
Before this patch, we didn't perform a PutHint of the Phi.
However, we need to, otherwise when exit, we won't know
the value of PromotedHeapLocation(@A, ActivationLoc)

The program we need then, for correctness, is this:
```
bb#0:
b: PhantomActivation()
a: PhantomNewFunction()
c: PutHint(@A, @b, ActivationLoc)
Branch(#1, #2)

bb#1:
d: MaterializeActivation()
e: PutHint(@A, @d, ActivationLoc)
f: Upsilon(@d, ^p)
Jump(#3)

bb#2:
g: MaterializeActivation()
h: PutHint(@A, @g, ActivationLoc)
i: Upsilon(@d, ^p)
Jump(#3)

bb#3:
p: Phi()
j: PutHint(@A, @p, ActivationLoc)
```

This patch makes it so that we emit the necessary PutHint at node `j`.
I've also added more validation to the OSRAvailabilityAnalysisPhase
to catch this problem during validation.

* dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
(JSC::DFG::OSRAvailabilityAnalysisPhase::run):
* dfg/DFGObjectAllocationSinkingPhase.cpp:
* ftl/FTLOperations.cpp:
(JSC::FTL::operationMaterializeObjectInOSR):


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

JSTests/ChangeLog

@@ -1,3 +1,20 @@
+2017-02-10  Saam Barati  <sbarati@apple.com>
+
+        Object allocation sinking phase doesn't properly handle control flow when emitting a PutHint of a materialized object into a PromotedHeapLocation of a still sunken object
+        https://bugs.webkit.org/show_bug.cgi?id=168140
+        <rdar://problem/30205880>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/allocation-sinking-puthint-control-flow.js: Added.
+        (e):
+        (bar):
+        (let.y):
+        (else.let.y):
+        (baz):
+        (foo):
+        (catch):
+
 2017-02-09  Mark Lam  <mark.lam@apple.com>
 
         B3::Procedure::deleteOrphans() should neutralize upsilons with dead phis.

JSTests/stress/allocation-sinking-puthint-control-flow.js

@@ -0,0 +1,41 @@
+function e() { }
+noInline(e);
+
+function foo(b, c, d) {
+    let x;
+    function bar() { return x; }
+    if (b) {
+        let y = function() { return x; }
+    } else {
+        let y = function() { return x; }
+    }
+
+    if (c) {
+        function baz() { }
+        if (b) {
+            let y = function() { return x; }
+            e(y);
+        } else {
+            let y = function() { return x; }
+            e(y);
+        }
+        if (d)
+            d();
+        e(baz);
+    }
+
+}
+noInline(foo);
+
+for (let i = 0; i < 100000; i++) {
+    foo(!!(i % 2), true, false);
+}
+
+let threw = false;
+try {
+    foo(true, true, true);
+} catch(e) {
+    threw = true;
+}
+if (!threw)
+    throw new Error("Bad test")

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,83 @@
+2017-02-10  Saam Barati  <sbarati@apple.com>
+
+        Object allocation sinking phase doesn't properly handle control flow when emitting a PutHint of a materialized object into a PromotedHeapLocation of a still sunken object
+        https://bugs.webkit.org/show_bug.cgi?id=168140
+        <rdar://problem/30205880>
+
+        Reviewed by Filip Pizlo.
+
+        This patch fixes a bug in allocation sinking phase where
+        we don't properly handle control flow when materializing
+        an object and also PutHinting that materialization into
+        a still sunken object. We were performing the PutHint
+        for the materialization at the point of materialization,
+        however, we may have materialized along both edges
+        of a control flow diamond, in which case, we need to
+        also PutHint at the join point. Consider this program:
+
+        ```
+        bb#0:
+        b: PhantomActivation()
+        a: PhantomNewFunction()
+        c: PutHint(@a, @b, ActivationLoc)
+        Branch(#1, #2)
+
+        bb#1:
+        d: MaterializeActivation()
+        e: PutHint(@a, @d, ActivationLoc)
+        f: Upsilon(@d, ^p)
+        Jump(#3)
+
+        bb#2:
+        g: MaterializeActivation()
+        h: PutHint(@a, @g, ActivationLoc)
+        i: Upsilon(@d, ^p)
+        Jump(#3)
+
+        bb#3:
+        p: Phi()
+        // What is PromotedHeapLocation(@a, ActivationLoc) here?
+        // What would we do if we exited?
+        ```
+        Before this patch, we didn't perform a PutHint of the Phi.
+        However, we need to, otherwise when exit, we won't know
+        the value of PromotedHeapLocation(@a, ActivationLoc)
+
+        The program we need then, for correctness, is this:
+        ```
+        bb#0:
+        b: PhantomActivation()
+        a: PhantomNewFunction()
+        c: PutHint(@a, @b, ActivationLoc)
+        Branch(#1, #2)
+
+        bb#1:
+        d: MaterializeActivation()
+        e: PutHint(@a, @d, ActivationLoc)
+        f: Upsilon(@d, ^p)
+        Jump(#3)
+
+        bb#2:
+        g: MaterializeActivation()
+        h: PutHint(@a, @g, ActivationLoc)
+        i: Upsilon(@d, ^p)
+        Jump(#3)
+
+        bb#3:
+        p: Phi()
+        j: PutHint(@a, @p, ActivationLoc)
+        ```
+
+        This patch makes it so that we emit the necessary PutHint at node `j`.
+        I've also added more validation to the OSRAvailabilityAnalysisPhase
+        to catch this problem during validation.
+
+        * dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
+        (JSC::DFG::OSRAvailabilityAnalysisPhase::run):
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+        * ftl/FTLOperations.cpp:
+        (JSC::FTL::operationMaterializeObjectInOSR):
+
 2017-02-10  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         WebInspector: refactor RemoteInspector to move cocoa specific code to their own files

Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp

@@ -95,9 +95,55 @@ class OSRAvailabilityAnalysisPhase : public Phase {
                 }
             }
         } while (changed);
+
+        if (validationEnabled()) {
+
+            for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex) {
+                BasicBlock* block = m_graph.block(blockIndex);
+                if (!block)
+                    continue;
+
+                calculator.beginBlock(block);
+
+                for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
+                    if (block->at(nodeIndex)->origin.exitOK) {
+                        // If we're allowed to exit here, the heap must be in a state
+                        // where exiting wouldn't crash. These particular fields are
+                        // required for correctness because we use them during OSR exit
+                        // to do meaningful things. It would be wrong for any of them
+                        // to be dead.
+
+                        AvailabilityMap availabilityMap = calculator.m_availability;
+                        availabilityMap.pruneByLiveness(m_graph, block->at(nodeIndex)->origin.forExit);
+
+                        for (auto heapPair : availabilityMap.m_heap) {
+                            switch (heapPair.key.kind()) {
+                            case ActivationScopePLoc:
+                            case ActivationSymbolTablePLoc:
+                            case FunctionActivationPLoc:
+                            case FunctionExecutablePLoc:
+                            case StructurePLoc:
+                                if (heapPair.value.isDead()) {
+                                    dataLogLn("PromotedHeapLocation is dead, but should not be: ", heapPair.key);
+                                    availabilityMap.dump(WTF::dataFile());
+                                    CRASH();
+                                }
+                                break;
+
+                            default:
+                                break;
+                            }
+                        }
+                    }
+
+                    calculator.executeNode(block->at(nodeIndex));
+                }
+            }
+        }
 
         return true;
     }
+
 };
 
 bool performOSRAvailabilityAnalysis(Graph& graph)

Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -1087,6 +1087,7 @@ class ObjectAllocationSinkingPhase : public Phase {
         m_materializationToEscapee.clear();
         m_materializationSiteToMaterializations.clear();
         m_materializationSiteToRecoveries.clear();
+        m_materializationSiteToHints.clear();
 
         // Logically we wish to consider every allocation and sink
         // it. However, it is probably not profitable to sink an
@@ -1242,16 +1243,35 @@ class ObjectAllocationSinkingPhase : public Phase {
             return;
 
         // First collect the hints that will be needed when the node
-        // we materialize is still stored into other unescaped sink candidates
-        Vector<PromotedHeapLocation> hints;
+        // we materialize is still stored into other unescaped sink candidates.
+        // The way to interpret this vector is:
+        //
+        // PromotedHeapLocation(NotEscapedAllocation, field) = identifierAllocation
+        //
+        // e.g:
+        // PromotedHeapLocation(@PhantomNewFunction, FunctionActivationPLoc) = IdentifierOf(@MaterializeCreateActivation)
+        // or:
+        // PromotedHeapLocation(@PhantomCreateActivation, ClosureVarPLoc(x)) = IdentifierOf(@NewFunction)
+        //
+        // When the rhs of the `=` is to be materialized at this `where` point in the program
+        // and IdentifierOf(Materialization) is the original sunken allocation of the materialization.
+        //
+        // The reason we need to collect all the `identifiers` here is that
+        // we may materialize multiple versions of the allocation along control
+        // flow edges. We will PutHint these values along those edges. However,
+        // we also need to PutHint them when we join and have a Phi of the allocations.
+        Vector<std::pair<PromotedHeapLocation, Node*>> hints;
         for (const auto& entry : m_heap.allocations()) {
             if (escapees.contains(entry.key))
                 continue;
 
             for (const auto& field : entry.value.fields()) {
                 ASSERT(m_sinkCandidates.contains(entry.key) || !escapees.contains(field.value));
-                if (escapees.contains(field.value))
-                    hints.append(PromotedHeapLocation(entry.key, field.key));
+                auto iter = escapees.find(field.value);
+                if (iter != escapees.end()) {
+                    ASSERT(m_sinkCandidates.contains(field.value));
+                    hints.append(std::make_pair(PromotedHeapLocation(entry.key, field.key), field.value));
+                }
             }
         }
 
@@ -1430,10 +1450,8 @@ class ObjectAllocationSinkingPhase : public Phase {
 
         // The hints need to be after the "real" recoveries so that we
         // don't hint not-yet-complete objects
-        if (!hints.isEmpty()) {
-            m_materializationSiteToRecoveries.add(
-                where, Vector<PromotedHeapLocation>()).iterator->value.appendVector(hints);
-        }
+        m_materializationSiteToHints.add(
+            where, Vector<std::pair<PromotedHeapLocation, Node*>>()).iterator->value.appendVector(hints);
     }
 
     Node* createMaterialization(const Allocation& allocation, Node* where)
@@ -1547,6 +1565,9 @@ class ObjectAllocationSinkingPhase : public Phase {
         HashMap<FrozenValue*, Node*> lazyMapping;
         if (!m_bottom)
             m_bottom = m_insertionSet.insertConstant(0, m_graph.block(0)->at(0)->origin, jsNumber(1927));
+
+        Vector<HashSet<PromotedHeapLocation>> hintsForPhi(m_sinkCandidates.size());
+
         for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
             m_heap = m_heapAtHead[block];
 
@@ -1568,6 +1589,31 @@ class ObjectAllocationSinkingPhase : public Phase {
                     m_allocationSSA.newDef(m_nodeToVariable.get(escapee), block, materialization);
                 }
 
+                for (std::pair<PromotedHeapLocation, Node*> pair : m_materializationSiteToHints.get(node)) {
+                    PromotedHeapLocation location = pair.first;
+                    Node* identifier = pair.second;
+                    // We're materializing `identifier` at this point, and the unmaterialized
+                    // version is inside `location`. We track which SSA variable this belongs
+                    // to in case we also need a PutHint for the Phi.
+                    if (UNLIKELY(validationEnabled())) {
+                        RELEASE_ASSERT(m_sinkCandidates.contains(location.base()));
+                        RELEASE_ASSERT(m_sinkCandidates.contains(identifier));
+
+                        bool found = false;
+                        for (Node* materialization : m_materializationSiteToMaterializations.get(node)) {
+                            // We're materializing `identifier` here. This asserts that this is indeed the case.
+                            if (m_materializationToEscapee.get(materialization) == identifier) {
+                                found = true;
+                                break;
+                            }
+                        }
+                        RELEASE_ASSERT(found);
+                    }
+
+                    SSACalculator::Variable* variable = m_nodeToVariable.get(identifier);
+                    hintsForPhi[variable->index()].add(location);
+                }
+
                 if (m_sinkCandidates.contains(node))
                     m_allocationSSA.newDef(m_nodeToVariable.get(node), block, node);
 
@@ -1683,6 +1729,12 @@ class ObjectAllocationSinkingPhase : public Phase {
                 insertOSRHintsForUpdate(
                     0, block->at(0)->origin, canExit,
                     availabilityCalculator.m_availability, identifier, phiDef->value());
+
+                for (PromotedHeapLocation location : hintsForPhi[variable->index()]) {
+                    m_insertionSet.insert(0,
+                        location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
+                    m_localMapping.set(location, phiDef->value());
+                }
             }
 
             if (verbose) {
@@ -1722,6 +1774,8 @@ class ObjectAllocationSinkingPhase : public Phase {
 
                 for (PromotedHeapLocation location : m_materializationSiteToRecoveries.get(node))
                     m_insertionSet.insert(nodeIndex, createRecovery(block, location, node, canExit));
+                for (std::pair<PromotedHeapLocation, Node*> pair : m_materializationSiteToHints.get(node))
+                    m_insertionSet.insert(nodeIndex, createRecovery(block, pair.first, node, canExit));
 
                 // We need to put the OSR hints after the recoveries,
                 // because we only want the hints once the object is
@@ -2209,6 +2263,7 @@ class ObjectAllocationSinkingPhase : public Phase {
     HashMap<Node*, Node*> m_materializationToEscapee;
     HashMap<Node*, Vector<Node*>> m_materializationSiteToMaterializations;
     HashMap<Node*, Vector<PromotedHeapLocation>> m_materializationSiteToRecoveries;
+    HashMap<Node*, Vector<std::pair<PromotedHeapLocation, Node*>>> m_materializationSiteToHints;
 
     HashMap<Node*, Vector<PromotedHeapLocation>> m_locationsForAllocation;
 

Source/JavaScriptCore/ftl/FTLOperations.cpp

@@ -132,6 +132,7 @@ extern "C" JSCell* JIT_OPERATION operationMaterializeObjectInOSR(
             if (property.location() != PromotedLocationDescriptor(StructurePLoc))
                 continue;
 
+            RELEASE_ASSERT(JSValue::decode(values[i]).asCell()->inherits(vm, Structure::info()));
             structure = jsCast<Structure*>(JSValue::decode(values[i]));
             break;
         }
@@ -163,10 +164,14 @@ extern "C" JSCell* JIT_OPERATION operationMaterializeObjectInOSR(
         JSScope* activation = nullptr;
         for (unsigned i = materialization->properties().size(); i--;) {
             const ExitPropertyValue& property = materialization->properties()[i];
-            if (property.location() == PromotedLocationDescriptor(FunctionExecutablePLoc))
+            if (property.location() == PromotedLocationDescriptor(FunctionExecutablePLoc)) {
+                RELEASE_ASSERT(JSValue::decode(values[i]).asCell()->inherits(vm, FunctionExecutable::info()));
                 executable = jsCast<FunctionExecutable*>(JSValue::decode(values[i]));
-            if (property.location() == PromotedLocationDescriptor(FunctionActivationPLoc))
+            }
+            if (property.location() == PromotedLocationDescriptor(FunctionActivationPLoc)) {
+                RELEASE_ASSERT(JSValue::decode(values[i]).asCell()->inherits(vm, JSScope::info()));
                 activation = jsCast<JSScope*>(JSValue::decode(values[i]));
+            }
         }
         RELEASE_ASSERT(executable && activation);
 
@@ -184,10 +189,13 @@ extern "C" JSCell* JIT_OPERATION operationMaterializeObjectInOSR(
         SymbolTable* table = nullptr;
         for (unsigned i = materialization->properties().size(); i--;) {
             const ExitPropertyValue& property = materialization->properties()[i];
-            if (property.location() == PromotedLocationDescriptor(ActivationScopePLoc))
+            if (property.location() == PromotedLocationDescriptor(ActivationScopePLoc)) {
+                RELEASE_ASSERT(JSValue::decode(values[i]).asCell()->inherits(vm, JSScope::info()));
                 scope = jsCast<JSScope*>(JSValue::decode(values[i]));
-            else if (property.location() == PromotedLocationDescriptor(ActivationSymbolTablePLoc))
+            } else if (property.location() == PromotedLocationDescriptor(ActivationSymbolTablePLoc)) {
+                RELEASE_ASSERT(JSValue::decode(values[i]).asCell()->inherits(vm, SymbolTable::info()));
                 table = jsCast<SymbolTable*>(JSValue::decode(values[i]));
+            }
         }
         RELEASE_ASSERT(scope);
         RELEASE_ASSERT(table);