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, saam, 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

Zan Dobersek
Reported 2017-12-26 08:56:46 PST
REGRESSION(r225913): about 30 JSC test failures on ARMv7
Attachments
Patch (1.87 KB, patch)
2017-12-26 09:01 PST, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.64 MB, application/zip)
2017-12-26 10:11 PST, EWS Watchlist 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Zan Dobersek
Comment 1 2017-12-26 09:01:06 PST
https://trac.webkit.org/changeset/225913
Zan Dobersek
Comment 2 2017-12-26 09:01:19 PST
Created attachment 330195 [details] Patch
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
Zan Dobersek
Comment 5 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>
Zan Dobersek
Comment 6 2017-12-27 02:52:45 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 7 2018-01-02 13:17:59 PST
<rdar://problem/36261349>
Darin Adler
Comment 8 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()).
Michael Catanzaro
Comment 9 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.
Keith Miller
Comment 10 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.
Zan Dobersek
Comment 11 2018-01-09 01:33:33 PST
Modified the comment and the if-statement condition in r226616. https://trac.webkit.org/r226616
Note You need to log in before you can comment on or make changes to this bug.