Bug 144525

Summary: DFG::LICMPhase should attempt to hoist edge type checks if hoisting the whole node fails
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, sbarati, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
WIP
none
patch
sbarati: review-
patch
none
Archive of layout-test-results from ews201 for win-future none

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>