[webkit-reviews] review granted: [Bug 228687] [JSC] Use loadPair / storePair in YarrJIT : [Attachment 434742] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 228687: [JSC] Use loadPair / storePair in YarrJIT
https://bugs.webkit.org/show_bug.cgi?id=228687

Attachment 434742: Patch

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




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 434742
  --> https://bugs.webkit.org/attachment.cgi?id=434742
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?