WebKit Bugzilla
Attachment 359948 Details for
Bug 193711
: [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-193711-20190123151257.patch (text/plain), 8.78 KB, created by
Yusuke Suzuki
on 2019-01-23 15:12:58 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-01-23 15:12:58 PST
Size:
8.78 KB
patch
obsolete
>Subversion Revision: 240297 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index c34ced4859fe3931fbbc2a2aa5f7cb5ce03beb94..234512ad5380f651469efb14d90168a98aceaff8 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,71 @@ >+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 >+ >+ 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..ad3f7c13b08cb533a5971559a02f7db8ac9bd9be 100644 >--- a/JSTests/ChangeLog >+++ b/JSTests/ChangeLog >@@ -1,3 +1,16 @@ >+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 >+ >+ 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);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193711
:
359865
|
359867
|
359868
|
359943
|
359948
|
359950