Bug 193711 – [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

This clears the lattice during fixed-point calculation.

[Yusuke Suzuki]

Created attachment 359865 [details]
Patch

[Yusuke Suzuki]

I think this is the issue, but I’m not 100% confident. I’ll ensure that tomorrow.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 359868 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 359943 [details]
ToT log for the given script

[Yusuke Suzuki]

Created attachment 359948 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/47250262>

[Yusuke Suzuki]

Created attachment 359950 [details]
Patch

[Saam Barati]

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")

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

Comment on attachment 359950 [details]
Patch

r=me

[Saam Barati]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r240364: <https://trac.webkit.org/changeset/240364>