Bug 193711 - [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
Summary: [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availabi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-23 02:25 PST by Yusuke Suzuki
Modified: 2019-01-23 16:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.97 KB, patch)
2019-01-23 02:43 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.66 MB, application/zip)
2019-01-23 03:27 PST, EWS Watchlist
no flags Details
Patch (1.47 KB, patch)
2019-01-23 03:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
ToT log for the given script (176.97 KB, text/plain)
2019-01-23 14:35 PST, Yusuke Suzuki
no flags Details
Patch (8.78 KB, patch)
2019-01-23 15:12 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.84 KB, patch)
2019-01-23 15:16 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-01-23 02:25:46 PST
This clears the lattice during fixed-point calculation.
Comment 1 Yusuke Suzuki 2019-01-23 02:43:49 PST
Created attachment 359865 [details]
Patch
Comment 2 Yusuke Suzuki 2019-01-23 02:52:56 PST
I think this is the issue, but I’m not 100% confident. I’ll ensure that tomorrow.
Comment 3 EWS Watchlist 2019-01-23 03:27:54 PST
Comment on attachment 359865 [details]
Patch

Attachment 359865 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10855417

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2019-01-23 03:27:56 PST
Created attachment 359867 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 5 Yusuke Suzuki 2019-01-23 03:45:10 PST
Created attachment 359868 [details]
Patch
Comment 6 Yusuke Suzuki 2019-01-23 12:28:44 PST
Let's consider the following example.

BB0 -> BB1 -> BB2 -> BB4
 |         \       ^
 v          > BB3 /
BB5

And consider about loc1 in FTL.

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 (b/c all the local operands are initialized with Conflicting)
BB1
DeadFlush (pruning clears it)
BB2
DeadFlush (since it is propagated from BB1)
BB3
FlushedJSValue with loc1 (since it has PutStack)
BB4
FlushedJSValue with loc1 (since DeadFlush + FlushedJSValue = FlushedJSValue)

Then, if we go the path BB0->BB1->BB2->BB4, we read the value from the stack while it is not flushed.

I think the correct fix is making availability "unavailable" when pruning based on the bytecode liveness.
Comment 7 Yusuke Suzuki 2019-01-23 14:35:24 PST
Created attachment 359943 [details]
ToT log for the given script
Comment 8 Yusuke Suzuki 2019-01-23 15:12:58 PST
Created attachment 359948 [details]
Patch
Comment 9 Yusuke Suzuki 2019-01-23 15:15:06 PST
<rdar://problem/47250262>
Comment 10 Yusuke Suzuki 2019-01-23 15:16:13 PST
Created attachment 359950 [details]
Patch
Comment 11 Saam Barati 2019-01-23 15:18:46 PST
Comment on attachment 359950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359950&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        When pruning OSR Availability based on bytecode liveness, we accidentally clears the Availability instead of making it Availability::unavailable().

Can you describe the difference please?

(Also, "clears" => "clear")
Comment 12 Saam Barati 2019-01-23 15:21:49 PST
Comment on attachment 359950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359950&action=review

> Source/JavaScriptCore/ChangeLog:46
> +        So, the flash format of loc1 in each tail of BB is like this.

flash => flush
Comment 13 Saam Barati 2019-01-23 15:23:13 PST
Comment on attachment 359950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359950&action=review

> Source/JavaScriptCore/ChangeLog:21
> +            BB0 does nothing
> +                head: loc1 is dead
> +                tail: loc1 is dead

I don't think this is interesting for this example to make sense.

> Source/JavaScriptCore/ChangeLog:41
> +            BB5 does nothing
> +                head: loc1 is dead
> +                tail: loc1 is dead

This isn't interesting for your example.
Comment 14 Saam Barati 2019-01-23 15:33:41 PST
Comment on attachment 359950 [details]
Patch

r=me
Comment 15 Saam Barati 2019-01-23 15:34:42 PST
Comment on attachment 359950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359950&action=review

>> Source/JavaScriptCore/ChangeLog:21
>> +                tail: loc1 is dead
> 
> I don't think this is interesting for this example to make sense.

Ignore me, this is required
Comment 16 Yusuke Suzuki 2019-01-23 15:47:00 PST
Comment on attachment 359950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359950&action=review

Thank you!

>> Source/JavaScriptCore/ChangeLog:9
>> +        When pruning OSR Availability based on bytecode liveness, we accidentally clears the Availability instead of making it Availability::unavailable().
> 
> Can you describe the difference please?
> 
> (Also, "clears" => "clear")

Fixed, and I described the difference between DeadFlush and ConflictingFlush.

>> Source/JavaScriptCore/ChangeLog:41
>> +                tail: loc1 is dead
> 
> This isn't interesting for your example.

This BB5 is required to split BB0 and BB1, and make BB1 non-root basic block. (because we do not clear the head's dead availability of the root block).

>> Source/JavaScriptCore/ChangeLog:46
>> +        So, the flash format of loc1 in each tail of BB is like this.
> 
> flash => flush

Fixed.
Comment 17 Yusuke Suzuki 2019-01-23 16:17:20 PST
Committed r240364: <https://trac.webkit.org/changeset/240364>