Bug 235112

Summary: AssemblyHelpersSpoolers: use load/store pair on ARMv7
Product: WebKit Reporter: Angelos Oikonomopoulos <angelos>
Component: New BugsAssignee: Angelos Oikonomopoulos <angelos>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glore, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Angelos Oikonomopoulos 2022-01-12 06:19:44 PST 
AssemblyHelpersSpoolers: use load/store pair on ARMv7
Comment 1 Angelos Oikonomopoulos 2022-01-12 06:20:14 PST 
Created attachment 448939 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2022-01-13 04:31:47 PST 
Created attachment 449044 [details]
Patch
Comment 3 Angelos Oikonomopoulos 2022-01-13 06:17:36 PST 
Created attachment 449054 [details]
Patch
Comment 4 Angelos Oikonomopoulos 2022-01-17 03:05:08 PST 
Created attachment 449322 [details]
Patch
Comment 5 Geza Lore 2022-01-17 04:14:50 PST 
Comment on attachment 449322 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:922
> +        ASSERT(src1 != src2);

This assertion is not necessary. Storing the same register twice is well defined, and works with storePair32.
Comment 6 Angelos Oikonomopoulos 2022-01-17 07:39:41 PST 
Created attachment 449333 [details]
Patch
Comment 7 Angelos Oikonomopoulos 2022-01-17 07:43:09 PST 
(In reply to Geza Lore from comment #5)
> Comment on attachment 449322 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449322&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:922
> > +        ASSERT(src1 != src2);
> 
> This assertion is not necessary. Storing the same register twice is well
> defined, and works with storePair32.

Ah. Removed, thanks!
Comment 8 Radar WebKit Bug Importer 2022-01-19 08:46:42 PST 
<rdar://problem/87772960>
Comment 9 Angelos Oikonomopoulos 2022-02-09 00:59:05 PST 
Ping.
Comment 10 Zan Dobersek 2022-03-04 06:37:08 PST 
Comment on attachment 449333 [details]
Patch

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

> Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:284
> +#if !(CPU(ARM64) || CPU(ARM))
> +        if constexpr (hasPairOp)
>              RELEASE_ASSERT_NOT_REACHED(); // unsupported architecture.
> +#endif

Honestly unguarded constexpr with additional `&& !isARM()` (if it existed) would be nicer.
Comment 11 Angelos Oikonomopoulos 2022-03-07 04:28:56 PST 
Created attachment 453963 [details]
Patch
Comment 12 Angelos Oikonomopoulos 2022-03-07 04:29:36 PST 
(In reply to Zan Dobersek from comment #10)
> Comment on attachment 449333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449333&action=review
> 
> > Source/JavaScriptCore/jit/AssemblyHelpersSpoolers.h:284
> > +#if !(CPU(ARM64) || CPU(ARM))
> > +        if constexpr (hasPairOp)
> >              RELEASE_ASSERT_NOT_REACHED(); // unsupported architecture.
> > +#endif
> 
> Honestly unguarded constexpr with additional `&& !isARM()` (if it existed)
> would be nicer.

Makes sense. Added and used.
Comment 13 EWS 2022-03-07 13:56:19 PST 
Committed r290907 (248137@main): <https://commits.webkit.org/248137@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453963 [details].