[Webkit-unassigned] [Bug 196080] [YARR] Improved JIT support on ARM/MIPS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=196080

--- Comment #17 from Michael Saboff <msaboff at apple.com> ---
Comment on attachment 366269
  --> https://bugs.webkit.org/attachment.cgi?id=366269
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190329/5c9eabb3/attachment.html>