[Webkit-unassigned] [Bug 196080] Support more paren expressions in RegExp-JIT on ARM/MIPS

[bugzilla-daemon at webkit.org]

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

--- Comment #11 from Dominik Inführ <dinfuehr at igalia.com> ---
(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.

-- 
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/20190323/db3a4b4e/attachment-0001.html>