Bug 228687 - [JSC] Use loadPair / storePair in YarrJIT
Summary: [JSC] Use loadPair / storePair in YarrJIT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-01 04:33 PDT by Yusuke Suzuki
Modified: 2021-08-02 19:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.23 KB, patch)
2021-08-01 04:37 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.78 KB, patch)
2021-08-01 04:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.68 KB, patch)
2021-08-01 04:50 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.08 KB, patch)
2021-08-01 05:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.05 KB, patch)
2021-08-01 12:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.01 KB, patch)
2021-08-01 21:20 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.18 KB, patch)
2021-08-01 21:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (41.89 KB, patch)
2021-08-01 22:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (42.17 KB, patch)
2021-08-01 22:23 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (43.33 KB, patch)
2021-08-01 22:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (44.72 KB, patch)
2021-08-02 00:26 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (35.57 KB, patch)
2021-08-02 00:37 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff
Patch (36.47 KB, patch)
2021-08-02 15:19 PDT, Yusuke Suzuki
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-08-01 04:33:31 PDT
[JSC] Use loadPair / storePair in YarrJIT
Comment 1 Yusuke Suzuki 2021-08-01 04:37:12 PDT
Created attachment 434716 [details]
Patch
Comment 2 Yusuke Suzuki 2021-08-01 04:47:16 PDT
Created attachment 434717 [details]
Patch
Comment 3 Yusuke Suzuki 2021-08-01 04:50:56 PDT
Created attachment 434718 [details]
Patch
Comment 4 Yusuke Suzuki 2021-08-01 05:07:25 PDT
Created attachment 434719 [details]
Patch
Comment 5 Mark Lam 2021-08-01 11:01:53 PDT
The test failures look real.  Can you check them please?
Comment 6 Yusuke Suzuki 2021-08-01 12:41:09 PDT
Created attachment 434725 [details]
Patch
Comment 7 Yusuke Suzuki 2021-08-01 12:41:18 PDT
Fixed!
Comment 8 Yusuke Suzuki 2021-08-01 21:20:57 PDT
Created attachment 434734 [details]
Patch
Comment 9 Yusuke Suzuki 2021-08-01 21:26:10 PDT
Created attachment 434735 [details]
Patch
Comment 10 Yusuke Suzuki 2021-08-01 22:18:35 PDT
Created attachment 434738 [details]
Patch
Comment 11 Yusuke Suzuki 2021-08-01 22:23:18 PDT
Created attachment 434739 [details]
Patch
Comment 12 Yusuke Suzuki 2021-08-01 22:28:33 PDT
Created attachment 434740 [details]
Patch
Comment 13 Yusuke Suzuki 2021-08-02 00:26:40 PDT
Created attachment 434741 [details]
Patch
Comment 14 Yusuke Suzuki 2021-08-02 00:37:23 PDT
Created attachment 434742 [details]
Patch
Comment 15 Mark Lam 2021-08-02 12:59:30 PDT
Comment on attachment 434742 [details]
Patch

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

r=me with suggestions.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1484
> +        if (offset.m_value <= 252 && offset.m_value >= -256) {

Let's use ARM64Assembler::isValidLDPImm<32>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1505
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidLDPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1526
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidLDPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1547
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidLDPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1883
> +        if (offset.m_value <= 252 && offset.m_value >= -256) {

Let's use ARM64Assembler::isValidSTPImm<32>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1898
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidSTPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1913
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidSTPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1928
> +        if (offset.m_value <= 504 && offset.m_value >= -512) {

Let's use ARM64Assembler::isValidSTPImm<64>() instead.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:877
> +        // FIXME: ldrd can be used if dest1 and dest2 are consecutive pair of registers.

ARMv7 can also use the ldm (load multiple) instruction.  Can you add that to the comment?

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:993
> +        // FIXME: strd can be used if src1 and src2 are consecutive pair of registers.

Can you add a comment here that ARMv7 can also use the stm (store multiple) instruction?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:302
>          static ptrdiff_t beginOffset()

Can this be constexpr?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:307
>          static ptrdiff_t matchAmountOffset()

Can this be constexpr?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:382
> +        ASSERT(ParenContext::beginOffset() + 4 == ParenContext::matchAmountOffset());

Can this be a static_assert?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:388
> +        ASSERT(ParenContext::beginOffset() + 4 == ParenContext::matchAmountOffset());

Can this be a static_assert?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:401
> -                loadPtr(Address(output, (subpattern << 1) * sizeof(unsigned)), tempReg);
> -                storePtr(tempReg, Address(parenContextReg, ParenContext::subpatternOffset(subpattern)));
> +                load64(Address(output, (subpattern << 1) * sizeof(unsigned)), tempReg);
> +                store64(tempReg, Address(parenContextReg, ParenContext::subpatternOffset(subpattern)));

How can this be correct on 32-bit platforms?  There's no load64 and store64, right?  Is this code compiled out on 32-bit?

Yep, looks like this is disabled on 32-bit:

#if ENABLE(YARR_JIT) && (CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)))
#define ENABLE_YARR_JIT_ALL_PARENS_EXPRESSIONS 1
#endif

Can you add a `static_assert(is64Bit())` here so that 32-bit doesn't fail silently if they ever enable this?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:422
> -                loadPtr(Address(parenContextReg, ParenContext::subpatternOffset(subpattern)), tempReg);
> -                storePtr(tempReg, Address(output, (subpattern << 1) * sizeof(unsigned)));
> +                load64(Address(parenContextReg, ParenContext::subpatternOffset(subpattern)), tempReg);
> +                store64(tempReg, Address(output, (subpattern << 1) * sizeof(unsigned)));

Ditto:  can you add a `static_assert(is64Bit())` here so that 32-bit doesn't fail silently if they ever enable this?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4062
> +    void loadSubPatterns(RegisterID outputGPR, unsigned subpatternId, RegisterID startIndexGPR, RegisterID endIndexOrLenGPR)

Maybe this should be `loadSubPattern` instead of `loadSubPatterns`.  Each pair of int32s contain data for only 1 subpattern, right?
Comment 16 Yusuke Suzuki 2021-08-02 15:15:47 PDT
Comment on attachment 434742 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1484
>> +        if (offset.m_value <= 252 && offset.m_value >= -256) {
> 
> Let's use ARM64Assembler::isValidLDPImm<32>() instead.

Sounds great! Fixed.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1505
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidLDPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1526
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidLDPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1547
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidLDPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1883
>> +        if (offset.m_value <= 252 && offset.m_value >= -256) {
> 
> Let's use ARM64Assembler::isValidSTPImm<32>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1898
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidSTPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1913
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidSTPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:1928
>> +        if (offset.m_value <= 504 && offset.m_value >= -512) {
> 
> Let's use ARM64Assembler::isValidSTPImm<64>() instead.

Done.

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:877
>> +        // FIXME: ldrd can be used if dest1 and dest2 are consecutive pair of registers.
> 
> ARMv7 can also use the ldm (load multiple) instruction.  Can you add that to the comment?

Added.

>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:993
>> +        // FIXME: strd can be used if src1 and src2 are consecutive pair of registers.
> 
> Can you add a comment here that ARMv7 can also use the stm (store multiple) instruction?

Added.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:302
>>          static ptrdiff_t beginOffset()
> 
> Can this be constexpr?

Changed.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:307
>>          static ptrdiff_t matchAmountOffset()
> 
> Can this be constexpr?

Changed.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:382
>> +        ASSERT(ParenContext::beginOffset() + 4 == ParenContext::matchAmountOffset());
> 
> Can this be a static_assert?

Changed.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:388
>> +        ASSERT(ParenContext::beginOffset() + 4 == ParenContext::matchAmountOffset());
> 
> Can this be a static_assert?

Changed.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:401
>> +                store64(tempReg, Address(parenContextReg, ParenContext::subpatternOffset(subpattern)));
> 
> How can this be correct on 32-bit platforms?  There's no load64 and store64, right?  Is this code compiled out on 32-bit?
> 
> Yep, looks like this is disabled on 32-bit:
> 
> #if ENABLE(YARR_JIT) && (CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)))
> #define ENABLE_YARR_JIT_ALL_PARENS_EXPRESSIONS 1
> #endif
> 
> Can you add a `static_assert(is64Bit())` here so that 32-bit doesn't fail silently if they ever enable this?

static_assert sounds good! Added.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:422
>> +                store64(tempReg, Address(output, (subpattern << 1) * sizeof(unsigned)));
> 
> Ditto:  can you add a `static_assert(is64Bit())` here so that 32-bit doesn't fail silently if they ever enable this?

Added.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:4062
>> +    void loadSubPatterns(RegisterID outputGPR, unsigned subpatternId, RegisterID startIndexGPR, RegisterID endIndexOrLenGPR)
> 
> Maybe this should be `loadSubPattern` instead of `loadSubPatterns`.  Each pair of int32s contain data for only 1 subpattern, right?

Right. Fixed.
Comment 17 Yusuke Suzuki 2021-08-02 15:19:47 PDT
Created attachment 434794 [details]
Patch
Comment 18 EWS 2021-08-02 18:45:17 PDT
Committed r280578 (240200@main): <https://commits.webkit.org/240200@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434794 [details].
Comment 19 Radar WebKit Bug Importer 2021-08-02 18:46:28 PDT
<rdar://problem/81439528>