diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog index a6dd7b0a4f89..dccf36c4a46e 100644 --- a/JSTests/ChangeLog +++ b/JSTests/ChangeLog @@ -1,3 +1,15 @@ +2020-01-21 Tadeu Zagallo + + Object allocation sinking is missing PutHint for allocations unreachable in the graph + https://bugs.webkit.org/show_bug.cgi?id=203799 + + + Reviewed by Saam Barati. + + * stress/allocation-sinking-puthint-control-flow-2.js: Added. + (f.handler.construct): + (f): + 2020-01-20 Gus Caplan Remove own toString from NativeError prototype diff --git a/JSTests/stress/allocation-sinking-puthint-control-flow-2.js b/JSTests/stress/allocation-sinking-puthint-control-flow-2.js new file mode 100644 index 000000000000..473de0652863 --- /dev/null +++ b/JSTests/stress/allocation-sinking-puthint-control-flow-2.js @@ -0,0 +1,16 @@ +//@ runDefault("--useConcurrentJIT=0", "--jitPolicyScale=0") + +function f() { + var x = {}; + x = 0; + var handler = { + construct: function () { + x; + } + }; + for (let i = 0; i < 1; i++) + (function () { i }); + new Proxy(function() { }, handler); +} +f(); +f(); diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index cfd882e91fd7..f1e35898e29a 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,44 @@ +2020-01-21 Tadeu Zagallo + + Object allocation sinking is missing PutHint for sunken allocations + https://bugs.webkit.org/show_bug.cgi?id=203799 + + + Reviewed by Saam Barati. + + Consider the following graph: + + Block #0: + 1: PhantomCreateActivation() + 2: PhantomNewFunction() + PutHint(@2, @1, FunctionActivationPLoc) + Branch(#1, #2) + + Block #1: + 3: MaterializeCreateActivation() + PutHint(@2, @3, FunctionActivationPLoc) + Upsilon(@3, ^5) + Jump(#3) + + Block #2: + 4: MaterializeCreateActivation() + PutHint(@2, @4, FunctionActivationPLoc) + Upsilon(@4, ^5) + Jump(#3) + + Block #3: + 5: Phi() + ExitOK() + + On Block #3, we need to emit a PutHint after the Phi, since we might exit after it. However, + object allocation sinking skipped this Phi because it was checking whether the base of the + location that caused us to create this Phi (@2) was live, but it's dead in the graph (there + are no pointers to it). The issue is that, even though there are no pointers to the base, the + location `PromotedHeapLocation(@2, FunctionActivationPLoc)` is still live, so we should PutHint + to it. We fix it by checking for liveness of the location rather than its base. + + * dfg/DFGObjectAllocationSinkingPhase.cpp: + 2020-01-21 Mark Lam Rename JSPromiseFields abstract heap to JSInternalFields. diff --git a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp index 0ea2315b5fc6..05acdf238769 100644 --- a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp @@ -410,7 +410,6 @@ class LocalHeap { { const Allocation& base = m_allocations.find(location.base())->value; auto iter = base.fields().find(location.descriptor()); - if (iter == base.fields().end()) return nullptr; @@ -438,6 +437,12 @@ class LocalHeap { return &getAllocation(identifier); } + bool isUnescapedAllocation(Node* identifier) const + { + auto iter = m_allocations.find(identifier); + return iter != m_allocations.end() && !iter->value.isEscapedAllocation(); + } + // This allows us to store the escapees only when necessary. If // set, the current escapees can be retrieved at any time using // takeEscapees(), which will clear the cached set of escapees; @@ -1946,7 +1951,7 @@ class ObjectAllocationSinkingPhase : public Phase { availabilityCalculator.m_availability, identifier, phiDef->value()); for (PromotedHeapLocation location : hintsForPhi[variable->index()]) { - if (m_heap.onlyLocalAllocation(location.base())) { + if (m_heap.isUnescapedAllocation(location.base())) { m_insertionSet.insert(0, location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value())); m_localMapping.set(location, phiDef->value());