Changeset 212177 in webkit for trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

Timestamp:

Feb 10, 2017, 8:05:06 PM (8 years ago)

Author:

[email protected]

Message:

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):

File:

• trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (9 diffs)

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -1088 +1088 @@
         m_materializationSiteToMaterializations.clear();
         m_materializationSiteToRecoveries.clear();
+        m_materializationSiteToHints.clear();
 
         // Logically we wish to consider every allocation and sink
@@ -1243 +1244 @@
 
         // 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))
@@ -1251 +1268 @@
             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));
+                }
             }
         }
@@ -1431 +1451 @@
         // 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);
     }
 
@@ -1548 +1566 @@
         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];
@@ -1567 +1588 @@
                     Node* escapee = m_materializationToEscapee.get(materialization);
                     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);
                 }
 
@@ -1684 +1730 @@
                     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());
+                }
             }
 
@@ -1723 +1775 @@
                 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,
@@ -2210 +2264 @@
     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;