Summary: | make css jit work on armv7 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
Component: | CSS | Assignee: | 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
Alex Christensen
2014-06-06 18:09:37 PDT
Created attachment 232641 [details]
Patch
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. Created attachment 232713 [details]
Patch
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. Created attachment 233060 [details]
Patch
(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. Created attachment 233062 [details]
Patch
Comment on attachment 233062 [details] Patch Clearing flags on attachment: 233062 Committed r169947: <http://trac.webkit.org/changeset/169947> All reviewed patches have been landed. Closing bug. 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. (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? :) |