Bug 196080 – [YARR] Improved JIT support on ARM/MIPS

• Status: NEW
• Component: New Bugs

[Dominik Inführ]

Support more paren expressions in RegExp-JIT on ARM/MIPS

[Dominik Inführ]

Created attachment 365554 [details]
Patch

[EWS Watchlist]

Attachment 365554 [details] did not pass style-queue:


ERROR: Source/WTF/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominik Inführ]

Created attachment 365555 [details]
Patch

[Caio Lima]

Comment on attachment 365555 [details]
Patch

LGTM

[Dominik Inführ]

Created attachment 365682 [details]
Patch

[EWS Watchlist]

Comment on attachment 365682 [details]
Patch

Attachment 365682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11610448

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365696 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Guillaume Emont]

Comment on attachment 365682 [details]
Patch

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

Did not test it, but the changes make sense to me, with a few nitpicks.

> Source/JavaScriptCore/assembler/MacroAssembler.h:585
> +    void addPtr(RegisterID src, Address address)

should address be called dest instead?

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:214
> +    void add32(RegisterID src, Address address)

Same question as for addPtr()

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3785
> +        pushPair(ARMRegisters::r11, ARMRegisters::r4); // push r4 twice for stack alignment

Would it cost more to do something like this? (for clarity sake):
  push(ArmRegisters::r11);
  sub32(stackPointerRegister, 4);

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3799
> +            move(TrustedImm32(0x10000), supplementaryPlanesBase);
> +            move(TrustedImm32(0xfffffc00), surrogateTagMask);
> +            move(TrustedImm32(0xd800), leadingSurrogateTag);
> +            move(TrustedImm32(0xdc00), trailingSurrogateTag);

shouldn't we have const variables or defines for these immediate values?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3851
> +        popPair(ARMRegisters::r11, ARMRegisters::r4); // pop r4 twice for stack alignment

see my comment when doing the matching pushPair().

> Source/WTF/wtf/Platform.h:1013
> +#if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS))

maybe an added pair of parentheses could make the meaning more obvious:
#if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || (OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS)))

[Dominik Inführ]

Created attachment 365746 [details]
Patch

[Dominik Inführ]

Created attachment 365748 [details]
Patch

[Dominik Inführ]

(In reply to Guillaume Emont from comment #8)
> Comment on attachment 365682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365682&action=review
> 
> Did not test it, but the changes make sense to me, with a few nitpicks.
> 
> > Source/JavaScriptCore/assembler/MacroAssembler.h:585
> > +    void addPtr(RegisterID src, Address address)
> 
> should address be called dest instead?
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:214
> > +    void add32(RegisterID src, Address address)
> 
> Same question as for addPtr()

Renamed.

> 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:3785
> > +        pushPair(ARMRegisters::r11, ARMRegisters::r4); // push r4 twice for stack alignment
> 
> Would it cost more to do something like this? (for clarity sake):
>   push(ArmRegisters::r11);
>   sub32(stackPointerRegister, 4);

I used pushPair here as well to be more in-line with the code before it.

> 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:3799
> > +            move(TrustedImm32(0x10000), supplementaryPlanesBase);
> > +            move(TrustedImm32(0xfffffc00), surrogateTagMask);
> > +            move(TrustedImm32(0xd800), leadingSurrogateTag);
> > +            move(TrustedImm32(0xdc00), trailingSurrogateTag);
> 
> shouldn't we have const variables or defines for these immediate values?

Maybe. Those constants were already duplicated for x64 and Arm64, I've now duplicated them for ARM and MIPS as well. It might now make sense to add constants for them. I will leave it as-is for now and see what the reviewer thinks.

> 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:3851
> > +        popPair(ARMRegisters::r11, ARMRegisters::r4); // pop r4 twice for stack alignment
> 
> see my comment when doing the matching pushPair().
> 
> > Source/WTF/wtf/Platform.h:1013
> > +#if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS))
> 
> maybe an added pair of parentheses could make the meaning more obvious:
> #if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || (OS(LINUX) &&
> (CPU(ARM_THUMB2) || CPU(MIPS)))

Changed.

[Dominik Inführ]

Created attachment 366259 [details]
Patch

[Dominik Inführ]

I've updated the patch for the changes made in https://bugs.webkit.org/show_bug.cgi?id=196296.

[Guillaume Emont]

Comment on attachment 366259 [details]
Patch

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

Changes still make sense to me, with the small nitpick of that typo. Again assuming that it runs and pass tests, as I did not check.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1256
> +    /* Need to use zero-extened load half-word for load16.  */

Typo here: s/extened/extended/

[Dominik Inführ]

Created attachment 366269 [details]
Patch

[Dominik Inführ]

(In reply to Guillaume Emont from comment #14)
> Comment on attachment 366259 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366259&action=review
> 
> Changes still make sense to me, with the small nitpick of that typo. Again
> assuming that it runs and pass tests, as I did not check.
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1256
> > +    /* Need to use zero-extened load half-word for load16.  */
> 
> Typo here: s/extened/extended/

Thanks, updated. I actually copied the description from another function and fixed the spelling there as well.

[Michael Saboff]

Comment on attachment 366269 [details]
Patch

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

Looks pretty good.  Got a few suggestions.

> Source/JavaScriptCore/assembler/MacroAssembler.h:588
> +
> +    void addPtr(RegisterID src, Address dest)
> +    {
> +        add32(src, dest);
> +    }

We probably don't need this.  See below.

> Source/JavaScriptCore/assembler/MacroAssembler.h:684
> +    void subPtr(TrustedImm32 imm, Address dest)
> +    {
> +        sub32(imm, dest);
> +    }
> +

We probably don't need this as well.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:220
> +    void add32(RegisterID src, Address dest)
> +    {
> +        load32(dest, dataTempRegister);
> +        add32(src, dataTempRegister);
> +        store32(dataTempRegister, dest);
> +    }
> +

We probably don't need this as well.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:57
> +    static const RegisterID initialStart = ARMRegisters::r14;
> +    static const RegisterID endOfStringAddress = ARMRegisters::fp;

These are the same register. Won't this mess up unwinding while in YarrJIT'ed code?  I see that you are saving lr on the stack, but I wonder what a debugger or crash trace shows for a backtrace.   I think you can safely eliminate a register for initialStart as it is only used for the .* wrapped optimization.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:269
>          addPtr(TrustedImm32(parenContextSize), freelistRegister, nextParenContextPointer);
> -        addPtr(freelistRegister, freelistSizeRegister);
> -        subPtr(TrustedImm32(parenContextSize), freelistSizeRegister);
> +        addPtr(freelistRegister, freelistSize);
> +        subPtr(TrustedImm32(parenContextSize), freelistSize);

What about restructuring this a little to eliminate the need for the addPtr(Reg, Address) and the subPtr(Imm, Address) macro instructions?  This would replace the load, add, store, load, sub, store sequence with a load, add, sub, store.  How about:

Move: "RegisterID nextParenContextPointer = regT2;" above to a new block for the init loop below and then restructure the calculation of the freelist end to:
        {
#if CPU(ARM_THUMB2)
            RegisterID freelistSizeRegister = regT2;
            loadPtr(freelistSize, freelistSizeRegister);
#endif
            addPtr(freelistRegister, freelistSizeRegister);
            subPtr(TrustedImm32(parenContextSize), freelistSizeRegister);
#if CPU(ARM_THUMB2)
            storePtr(freelistSizeRegister, freelistSize);
#endif
        }

Turn the free list initialization loop to:
        {
            RegisterID nextParenContextPointer = regT2;
            addPtr(TrustedImm32(parenContextSize), freelistRegister, nextParenContextPointer);
            ...
            jump(loopTop);
        }

> Source/JavaScriptCore/yarr/YarrJIT.cpp:701
> +        m_callFrameSizeInBytes = alignCallFrameSizeInBytes(m_pattern.m_body->m_callFrameSize);

Let's move this member initialization to the constructor.