Bug 133596

Summary: make css jit work on armv7
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: CSSAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ossy, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2014-06-06 18:09:37 PDT
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.
Comment 1 Alex Christensen 2014-06-06 18:24:37 PDT
Created attachment 232641 [details]
Patch
Comment 2 Yusuke Suzuki 2014-06-07 10:20:44 PDT
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.
Comment 3 Alex Christensen 2014-06-09 10:54:15 PDT
Created attachment 232713 [details]
Patch
Comment 4 Benjamin Poulain 2014-06-10 16:38:26 PDT
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.
Comment 5 Alex Christensen 2014-06-13 10:58:06 PDT
Created attachment 233060 [details]
Patch
Comment 6 Alex Christensen 2014-06-13 11:12:07 PDT
(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.
Comment 7 Alex Christensen 2014-06-13 11:15:50 PDT
Created attachment 233062 [details]
Patch
Comment 8 WebKit Commit Bot 2014-06-13 13:44:02 PDT
Comment on attachment 233062 [details]
Patch

Clearing flags on attachment: 233062

Committed r169947: <http://trac.webkit.org/changeset/169947>
Comment 9 WebKit Commit Bot 2014-06-13 13:44:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogon√°c 2014-06-14 02:14:25 PDT
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.
Comment 11 Benjamin Poulain 2014-06-14 13:58:55 PDT
(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? :)