Bug 144525 - DFG::LICMPhase should attempt to hoist edge type checks if hoisting the whole node fails
Summary: DFG::LICMPhase should attempt to hoist edge type checks if hoisting the whole...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 185795 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-05-02 10:26 PDT by Filip Pizlo
Modified: 2018-05-22 12:49 PDT (History)
15 users (show)

See Also:


Attachments
WIP (12.17 KB, patch)
2018-05-20 23:05 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (14.73 KB, patch)
2018-05-21 00:56 PDT, Saam Barati
sbarati: review-
Details | Formatted Diff | Diff
patch (15.51 KB, patch)
2018-05-21 11:21 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (12.75 MB, application/zip)
2018-05-21 22:05 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-05-02 10:26:52 PDT
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.
Comment 1 Saam Barati 2018-05-20 22:47:13 PDT
*** Bug 185795 has been marked as a duplicate of this bug. ***
Comment 2 Saam Barati 2018-05-20 23:05:06 PDT
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.
Comment 3 Saam Barati 2018-05-21 00:56:03 PDT
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.
Comment 4 Saam Barati 2018-05-21 11:10:56 PDT
Comment on attachment 340832 [details]
patch

This doesn't do check hoisting on nodes that write. That's wrong...
Comment 5 Saam Barati 2018-05-21 11:21:17 PDT
Created attachment 340865 [details]
patch
Comment 6 Saam Barati 2018-05-21 11:47:16 PDT
Benchmarks seem neutral. It may be a speedup in one or two subtests in Octane. Going to run them again to verify.
Comment 7 Saam Barati 2018-05-21 12:01:17 PDT
(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.
Comment 8 EWS Watchlist 2018-05-21 22:05:01 PDT
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
Comment 9 EWS Watchlist 2018-05-21 22:05:12 PDT
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
Comment 10 Filip Pizlo 2018-05-22 10:07:23 PDT
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.
Comment 11 Saam Barati 2018-05-22 12:20:39 PDT
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 :-)
Comment 12 WebKit Commit Bot 2018-05-22 12:47:45 PDT
Comment on attachment 340865 [details]
patch

Clearing flags on attachment: 340865

Committed r232075: <https://trac.webkit.org/changeset/232075>
Comment 13 WebKit Commit Bot 2018-05-22 12:47:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-05-22 12:49:10 PDT
<rdar://problem/40460830>