Attachment 359950 Details for Bug 193711 – Patch

[patch] Patch

bug-193711-20190123151613.patch (text/plain), 8.84 KB, created by Yusuke Suzuki on 2019-01-23 15:16:13 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-23 15:16:13 PST

Size: 8.84 KB

patch

obsolete

>Subversion Revision: 240297
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c34ced4859fe3931fbbc2a2aa5f7cb5ce03beb94..96197eb1019a2846000e9b1dc370bdc8a77201f8 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,72 @@
>+2019-01-23  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
>+        https://bugs.webkit.org/show_bug.cgi?id=193711
>+        <rdar://problem/47250262>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When pruning OSR Availability based on bytecode liveness, we accidentally clears the Availability instead of making it Availability::unavailable().
>+        Let's consider the following simple example. We have 6 basic blocks, and they are connected as follows.
>+
>+            BB0 -> BB1 -> BB2 -> BB4
>+             |         \       ^
>+              v          > BB3 /
>+              BB5
>+
>+        And consider about loc1 in FTL, which is required to be recovered in BB4's OSR exit.
>+
>+            BB0 does nothing
>+                head: loc1 is dead
>+                tail: loc1 is dead
>+
>+            BB1 has MovHint @1, loc1
>+                head: loc1 is dead
>+                tail: loc1 is live
>+
>+            BB2 does nothing
>+                head: loc1 is live
>+                tail: loc1 is live
>+
>+            BB3 has PutStack @1, loc1
>+                head: loc1 is live
>+                tail: loc1 is live
>+
>+            BB4 has OSR exit using loc1
>+                head: loc1 is live
>+                tail: loc1 is live (in bytecode)
>+
>+            BB5 does nothing
>+                head: loc1 is dead
>+                tail: loc1 is dead
>+
>+        In our OSR Availability analysis, we always prune loc1 result in BB1's head since its head says "loc1 is dead".
>+        But at that time, we clear the availability for loc1, which makes it DeadFlush, instead of making it Conflicting.
>+
>+        So, the flash format of loc1 in each tail of BB is like this.
>+
>+            BB0
>+                Conflicting (because all the local operands are initialized with Conflicting)
>+            BB1
>+                DeadFlush+@1 (pruning clears it)
>+            BB2
>+                DeadFlush+@1 (since it is propagated from BB1)
>+            BB3
>+                FlushedJSValue+@1 with loc1 (since it has PutStack)
>+            BB4
>+                FlushedJSValue+@1 with loc1 (since MERGE(DeadFlush, FlushedJSValue) = FlushedJSValue)
>+            BB5
>+                DeadFlush (pruning clears it)
>+
>+        Then, if we go the path BB0->BB1->BB2->BB4, we read the value from the stack while it is not flushed.
>+        The correct fix is making availability "unavailable" when pruning based on bytecode liveness.
>+
>+        * dfg/DFGAvailabilityMap.cpp:
>+        (JSC::DFG::AvailabilityMap::pruneByLiveness): When pruning availability, we first set all the operands Availability::unavailable(),
>+        and copy the calculated value from the current availability map.
>+        * dfg/DFGOSRAvailabilityAnalysisPhase.cpp:
>+        (JSC::DFG::OSRAvailabilityAnalysisPhase::run): Add logging things for debugging.
>+
> 2019-01-22  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Intl constructors should fit in sizeof(InternalFunction)
>diff --git a/Source/JavaScriptCore/dfg/DFGAvailabilityMap.cpp b/Source/JavaScriptCore/dfg/DFGAvailabilityMap.cpp
>index 7855f8923506c16ca9cc834e03574d5b5a09651e..7743e7d01f1cad781d84ab03d3c09c77f8029ba5 100644
>--- a/Source/JavaScriptCore/dfg/DFGAvailabilityMap.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGAvailabilityMap.cpp
>@@ -65,7 +65,7 @@ void AvailabilityMap::pruneHeap()
> 
> void AvailabilityMap::pruneByLiveness(Graph& graph, CodeOrigin where)
> {
>-    Operands<Availability> localsCopy(OperandsLike, m_locals);
>+    Operands<Availability> localsCopy(m_locals.numberOfArguments(), m_locals.numberOfLocals(), Availability::unavailable());
>     graph.forAllLiveInBytecode(
>         where,
>         [&] (VirtualRegister reg) {
>diff --git a/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp b/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
>index fb37153d08646881538dc9c6b1a8e1fedddd9616..18b14c15aea64eb16a24bb41d99c915e861f4be2 100644
>--- a/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp
>@@ -35,6 +35,9 @@
> #include "JSCInlines.h"
> 
> namespace JSC { namespace DFG {
>+namespace DFGOSRAvailabilityAnalysisPhaseInternal {
>+static constexpr bool verbose = false;
>+}
> 
> class OSRAvailabilityAnalysisPhase : public Phase {
> public:
>@@ -63,6 +66,21 @@ class OSRAvailabilityAnalysisPhase : public Phase {
> 
>         // This could be made more efficient by processing blocks in reverse postorder.
>         
>+        auto dumpAvailability = [] (BasicBlock* block) {
>+            dataLogLn(block->ssa->availabilityAtHead);
>+            dataLogLn(block->ssa->availabilityAtTail);
>+        };
>+
>+        auto dumpBytecodeLivenessAtHead = [&] (BasicBlock* block) {
>+            dataLog("Live: ");
>+            m_graph.forAllLiveInBytecode(
>+                block->at(0)->origin.forExit,
>+                [&] (VirtualRegister reg) {
>+                    dataLog(reg, " ");
>+                });
>+            dataLogLn("");
>+        };
>+
>         LocalOSRAvailabilityCalculator calculator(m_graph);
>         bool changed;
>         do {
>@@ -73,6 +91,10 @@ class OSRAvailabilityAnalysisPhase : public Phase {
>                 if (!block)
>                     continue;
>                 
>+                if (DFGOSRAvailabilityAnalysisPhaseInternal::verbose) {
>+                    dataLogLn("Before changing Block #", block->index);
>+                    dumpAvailability(block);
>+                }
>                 calculator.beginBlock(block);
>                 
>                 for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex)
>@@ -84,6 +106,11 @@ class OSRAvailabilityAnalysisPhase : public Phase {
>                 block->ssa->availabilityAtTail = calculator.m_availability;
>                 changed = true;
> 
>+                if (DFGOSRAvailabilityAnalysisPhaseInternal::verbose) {
>+                    dataLogLn("After changing Block #", block->index);
>+                    dumpAvailability(block);
>+                }
>+
>                 for (unsigned successorIndex = block->numSuccessors(); successorIndex--;) {
>                     BasicBlock* successor = block->successor(successorIndex);
>                     successor->ssa->availabilityAtHead.merge(calculator.m_availability);
>@@ -93,6 +120,11 @@ class OSRAvailabilityAnalysisPhase : public Phase {
>                     BasicBlock* successor = block->successor(successorIndex);
>                     successor->ssa->availabilityAtHead.pruneByLiveness(
>                         m_graph, successor->at(0)->origin.forExit);
>+                    if (DFGOSRAvailabilityAnalysisPhaseInternal::verbose) {
>+                        dataLogLn("After pruning Block #", successor->index);
>+                        dumpAvailability(successor);
>+                        dumpBytecodeLivenessAtHead(successor);
>+                    }
>                 }
>             }
>         } while (changed);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index d855bd472dafbaace71fd7ec2216c2e119d52cc3..5b43d2201092534567c9b887c68ca77e4abf4f02 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-23  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
>+        https://bugs.webkit.org/show_bug.cgi?id=193711
>+        <rdar://problem/47250262>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/availability-was-cleared-when-locals-are-not-live.js: Added.
>+        (shouldBe):
>+        (foo):
>+        (bar):
>+        (baz):
>+
> 2019-01-22  Saam Barati  <sbarati@apple.com>
> 
>         Unreviewed. Rollout r240223. It regressed JetStream2 by 1%.
>diff --git a/JSTests/stress/availability-was-cleared-when-locals-are-not-live.js b/JSTests/stress/availability-was-cleared-when-locals-are-not-live.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..f9558ed1719019521b0374aa6544d4d5a01402d0
>--- /dev/null
>+++ b/JSTests/stress/availability-was-cleared-when-locals-are-not-live.js
>@@ -0,0 +1,37 @@
>+//@ runDefault("--jitPolicyScale=0", "--useConcurrentJIT=false")
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+noInline(shouldBe);
>+
>+var a;
>+
>+function foo(x, y, z) {
>+    baz(a);
>+    0 + (x ? a : [] + 0);
>+    return y;
>+}
>+
>+function bar() {
>+    return foo.apply(null, arguments);
>+}
>+
>+function baz(p) {
>+    if (p) {
>+        return bar(1, 1, 0);
>+    }
>+}
>+
>+baz(1);
>+
>+for (let i = 0; i < 1; i++) {
>+    foo(1);
>+}
>+
>+for (let i = 0; i < 10000; i++) {
>+    baz();
>+}
>+
>+let hello = baz(1);
>+shouldBe(hello, 1);

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193711: 359865 | 359867 | 359868 | 359943 | 359948 | 359950