Bug 133596 – make css jit work on armv7

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alex Christensen]

I haven't tested this code on a device and there are some hacky things, but I think this is a big step in the right direction.

[Alex Christensen]

Created attachment 232641 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 232641 [details]
Patch

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

Great patch!

> Source/WebCore/cssjit/SelectorCompiler.cpp:1474
> +    m_assembler.m_assembler.sdiv(resultRegister, inputDividend, divisorRegister);

IIRC, APPLE_ARMV7S (ARMv7s) supports sdiv, but ARMv7 doesn't support it. So I think fallback path is needed.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1866
> +#endif

Calculating element pointer + member's offset to lookup struct's member.
So using addPtr (or addPtrNoFlags) instead and we can remove this ifdef :)

> Source/WebCore/cssjit/StackAllocator.h:220
> +#endif

`MacroAssembler::pushToSaveByteOffset` implementation for ARM_THUMB2 is `sizeof(void*)` and I think this ifdef is not necessary.

[Alex Christensen]

Created attachment 232713 [details]
Patch

[Benjamin Poulain]

Comment on attachment 232713 [details]
Patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1415
> +        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/

I don't understand the problem here. Why isn't addressTempRegister the right register in this case?

> Source/WebCore/cssjit/FunctionCall.h:94
> +#elif CPU(ARM_THUMB2)

What about MIPS? ;)

> Source/WebCore/cssjit/RegisterAllocator.h:67
> +    JSC::ARMRegisters::r6,

We cannot use r6, it could be used for addressTempRegister.

I believe r4->r6 are callee saved registers ( https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW10 )

> Source/WebCore/cssjit/SelectorCompiler.cpp:1095
> +    prologueRegisters.append(JSC::ARMRegisters::fp);

Do we care about fp?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1138
> +    // ARM_THUMB2 does not have enough registers to compile complicated selectors.
> +    // Compiling should always succeed on non-ARM_THUMB2 CPUs.
> +    ASSERT_NOT_REACHED();

Indent.

[Alex Christensen]

Created attachment 233060 [details]
Patch

[Alex Christensen]

(In reply to comment #4)
> (From update of attachment 232713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232713&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1415
> > +        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/
> 
> I don't understand the problem here. Why isn't addressTempRegister the right register in this case?
I don't know.  This was me making the code look like the code around it without fully understanding what the registers are used for.
> Do we care about fp?
I use fp as a caller saved register, and pushing 2 registers takes the same number of operations as pushing one.  See comments in new patch, which will remind us to change this if we optimize for selectors that don't do function calls that won't need a prologue or epilogue.

This works and passes all fast/selectors tests on armv7s.

[Alex Christensen]

Created attachment 233062 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 233062 [details]
Patch

Clearing flags on attachment: 233062

Committed r169947: <http://trac.webkit.org/changeset/169947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 233062 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:1405
> +#else
> +#error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.
> +#endif

It broke the EFL ARM64 build:

/home/webkitbuildbot/slaves/efl-ARM64-build/buildslave/efl-linux-arm64-release/build/Source/WebCore/cssjit/SelectorCompiler.cpp:1404:2: error: #error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.
 #error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.

[Benjamin Poulain]

(In reply to comment #10)
> (From update of attachment 233062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233062&action=review
> 
> > Source/WebCore/cssjit/SelectorCompiler.cpp:1405
> > +#else
> > +#error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.
> > +#endif
> 
> It broke the EFL ARM64 build:
> 
> /home/webkitbuildbot/slaves/efl-ARM64-build/buildslave/efl-linux-arm64-release/build/Source/WebCore/cssjit/SelectorCompiler.cpp:1404:2: error: #error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.
>  #error SelectorCodeGenerator::addFlagsToElementStyleFromContext not implemented for this architecture.

Quick fix (hopefully): <http://trac.webkit.org/changeset/169980>.

Any chance to get a ARM64 EWS some day? :)