Bug 144525 – DFG::LICMPhase should attempt to hoist edge type checks if hoisting the whole node fails

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

This usually isn't super necessary because the CPS type check hoisting - done in FixupPhase by putting checks on SetLocals - does a pretty good job.  But it only covers the super common types and sometimes it will not do anything if the VariableAccessData looks sketchy.  So, LICMPhase does have an opportunity here for some small additional speed-up.

[Saam Barati]

*** Bug 185795 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 340829 [details]
WIP

Seems to hoist some stuff in Octane. Need to clean some things up before actually putting it up for review.

[Saam Barati]

Created attachment 340832 [details]
patch

Will run some benchmarks tomorrow before landing. I've verified via logging that various checks do get hoisted in Octane.

[Saam Barati]

Comment on attachment 340832 [details]
patch

This doesn't do check hoisting on nodes that write. That's wrong...

[Saam Barati]

Created attachment 340865 [details]
patch

[Saam Barati]

Benchmarks seem neutral. It may be a speedup in one or two subtests in Octane. Going to run them again to verify.

[Saam Barati]

(In reply to Saam Barati from comment #6)
> Benchmarks seem neutral. It may be a speedup in one or two subtests in
> Octane. Going to run them again to verify.

Seems like a 3-4% speedup on Octane/regexp. Seems neutral on most other things, with some maybe 1% speedups sprinkled about. I think the bots will be the best source of truth for this change.

[EWS Watchlist]

Comment on attachment 340865 [details]
patch

Attachment 340865 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7761006

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 340953 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Filip Pizlo]

Comment on attachment 340865 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGUseKind.h:310
> +    case Int32Use:
> +    case KnownInt32Use:
> +    case AnyIntUse:
> +    case NumberUse:

Why do we say that these crash when the input is empty?

Int32 check will only pass if the input is non-empty.  Probably ditto for a lot of these.

[Saam Barati]

Comment on attachment 340865 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGUseKind.h:310
>> +    case NumberUse:
> 
> Why do we say that these crash when the input is empty?
> 
> Int32 check will only pass if the input is non-empty.  Probably ditto for a lot of these.

We're returning false here, so the opposite of crashing :-)

[WebKit Commit Bot]

Comment on attachment 340865 [details]
patch

Clearing flags on attachment: 340865

Committed r232075: <https://trac.webkit.org/changeset/232075>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40460830>