Bug 228687 – [JSC] Use loadPair / storePair in YarrJIT

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use loadPair / storePair in YarrJIT

[Yusuke Suzuki]

Created attachment 434716 [details]
Patch

[Yusuke Suzuki]

Created attachment 434717 [details]
Patch

[Yusuke Suzuki]

Created attachment 434718 [details]
Patch

[Yusuke Suzuki]

Created attachment 434719 [details]
Patch

[Mark Lam]

The test failures look real.  Can you check them please?

[Yusuke Suzuki]

Created attachment 434725 [details]
Patch

[Yusuke Suzuki]

Fixed!

[Yusuke Suzuki]

Created attachment 434734 [details]
Patch

[Yusuke Suzuki]

Created attachment 434735 [details]
Patch

[Yusuke Suzuki]

Created attachment 434738 [details]
Patch

[Yusuke Suzuki]

Created attachment 434739 [details]
Patch

[Yusuke Suzuki]

Created attachment 434740 [details]
Patch

[Yusuke Suzuki]

Created attachment 434741 [details]
Patch

[Yusuke Suzuki]

Created attachment 434742 [details]
Patch

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 434794 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81439528>