Bug 181162

Summary: REGRESSION(r225913): about 30 JSC test failures on ARMv7
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, ossy, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 none

Description Zan Dobersek 2017-12-26 08:56:46 PST
REGRESSION(r225913): about 30 JSC test failures on ARMv7
Comment 1 Zan Dobersek 2017-12-26 09:01:06 PST
https://trac.webkit.org/changeset/225913
Comment 2 Zan Dobersek 2017-12-26 09:01:19 PST
Created attachment 330195 [details]
Patch
Comment 3 EWS Watchlist 2017-12-26 10:11:33 PST
Comment on attachment 330195 [details]
Patch

Attachment 330195 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5836010

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html
Comment 4 EWS Watchlist 2017-12-26 10:11:34 PST
Created attachment 330199 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Zan Dobersek 2017-12-27 02:52:41 PST
Comment on attachment 330195 [details]
Patch

Clearing flags on attachment: 330195

Committed r226298: <https://trac.webkit.org/changeset/226298>
Comment 6 Zan Dobersek 2017-12-27 02:52:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-01-02 13:17:59 PST
<rdar://problem/36261349>
Comment 8 Darin Adler 2018-01-07 16:58:04 PST
Comment on attachment 330195 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7638
>          // X86 only has 6 GP registers, which is not enough for the fast case here. At least without custom code, which is not currently worth the extra code maintenance.
> -        if (!isX86() || isX86_64()) {
> +        if (isARM64() || isX86_64()) {

The code change invalidates the comment; so it should have updated the comment.

Also seems slightly wrong to require anyone adding a future 64-bit target to this if statement. The old code was X86-specific so would do no harm for non-X86. The new code means we only get this optimization for explicitly listed platforms. Would be slightly nicer if this could be written in a more direct way, like if (!tooFewGPRegisters()).
Comment 9 Michael Catanzaro 2018-01-07 17:05:59 PST
Reopening for Zan... I should have spotted that the comment was stale, sorry. At least that should be fixed.
Comment 10 Keith Miller 2018-01-08 16:30:13 PST
Comment on attachment 330195 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7638
>> +        if (isARM64() || isX86_64()) {
> 
> The code change invalidates the comment; so it should have updated the comment.
> 
> Also seems slightly wrong to require anyone adding a future 64-bit target to this if statement. The old code was X86-specific so would do no harm for non-X86. The new code means we only get this optimization for explicitly listed platforms. Would be slightly nicer if this could be written in a more direct way, like if (!tooFewGPRegisters()).

I think you can just do `if (is64Bit())`. That's assuming all 64 bit platforms have enough registers for this, which I think is the case.
Comment 11 Zan Dobersek 2018-01-09 01:33:33 PST
Modified the comment and the if-statement condition in r226616.
https://trac.webkit.org/r226616