Title: [254725] trunk
Revision
254725
Author
tzaga...@apple.com
Date
2020-01-16 16:55:15 -0800 (Thu, 16 Jan 2020)

Log Message

JSTests:
Object allocation sinking is missing PutHint for allocations unreachable in the graph
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

Reviewed by Saam Barati.

* stress/allocation-sinking-puthint-control-flow-2.js: Added.
(f.handler.construct):
(f):

Source/_javascript_Core:
Object allocation sinking is missing PutHint for sunken allocations
https://bugs.webkit.org/show_bug.cgi?id=203799
<rdar://problem/56852162>

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:

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (254724 => 254725)


--- trunk/JSTests/ChangeLog	2020-01-17 00:54:06 UTC (rev 254724)
+++ trunk/JSTests/ChangeLog	2020-01-17 00:55:15 UTC (rev 254725)
@@ -1,3 +1,15 @@
+2020-01-16  Tadeu Zagallo  <tzaga...@apple.com>
+
+        Object allocation sinking is missing PutHint for allocations unreachable in the graph
+        https://bugs.webkit.org/show_bug.cgi?id=203799
+        <rdar://problem/56852162>
+
+        Reviewed by Saam Barati.
+
+        * stress/allocation-sinking-puthint-control-flow-2.js: Added.
+        (f.handler.construct):
+        (f):
+
 2020-01-16  Robin Morisset  <rmoris...@apple.com>
 
         Teach the bytecode that arithmetic operations can return bigints

Added: trunk/JSTests/stress/allocation-sinking-puthint-control-flow-2.js (0 => 254725)


--- trunk/JSTests/stress/allocation-sinking-puthint-control-flow-2.js	                        (rev 0)
+++ trunk/JSTests/stress/allocation-sinking-puthint-control-flow-2.js	2020-01-17 00:55:15 UTC (rev 254725)
@@ -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();

Modified: trunk/Source/_javascript_Core/ChangeLog (254724 => 254725)


--- trunk/Source/_javascript_Core/ChangeLog	2020-01-17 00:54:06 UTC (rev 254724)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-01-17 00:55:15 UTC (rev 254725)
@@ -1,3 +1,44 @@
+2020-01-16  Tadeu Zagallo  <tzaga...@apple.com>
+
+        Object allocation sinking is missing PutHint for sunken allocations
+        https://bugs.webkit.org/show_bug.cgi?id=203799
+        <rdar://problem/56852162>
+
+        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-16  Robin Morisset  <rmoris...@apple.com>
 
         Try to simplify the template deduction used by callOperation in DFGSpeculativeJIT

Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (254724 => 254725)


--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp	2020-01-17 00:54:06 UTC (rev 254724)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp	2020-01-17 00:55:15 UTC (rev 254725)
@@ -409,8 +409,10 @@
     Node* follow(PromotedHeapLocation location) const
     {
         const Allocation& base = m_allocations.find(location.base())->value;
+        if (base.isEscapedAllocation())
+            return nullptr;
+
         auto iter = base.fields().find(location.descriptor());
-
         if (iter == base.fields().end())
             return nullptr;
 
@@ -1946,7 +1948,7 @@
                     availabilityCalculator.m_availability, identifier, phiDef->value());
 
                 for (PromotedHeapLocation location : hintsForPhi[variable->index()]) {
-                    if (m_heap.onlyLocalAllocation(location.base())) {
+                    if (m_heap.onlyLocalAllocation(location)) {
                         m_insertionSet.insert(0,
                             location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
                         m_localMapping.set(location, phiDef->value());
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to