[Webkit-unassigned] [Bug 133156] make css jit work on arm64

[bugzilla-daemon at webkit.org]

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


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #231918|review?                     |review-
               Flag|                            |




--- Comment #8 from Benjamin Poulain <benjamin at webkit.org>  2014-05-22 17:53:23 PST ---
(From update of attachment 231918)
View in context: https://bugs.webkit.org/attachment.cgi?id=231918&action=review

Great encapsulation for the ARM64 parts!

Some comments bellow and I have a big doubt with the stack references.

> Source/WebCore/cssjit/FunctionCall.h:168
> +        m_savedRegisterStackReferences.appendVector(m_stackAllocator.push(m_registerAllocator.allocatedRegisters()));

Can you split this on 3 lines?
It is 3 important operations, I would prefer to have 3 statements.

> Source/WebCore/cssjit/RegisterAllocator.h:-32
> -#include <StackAllocator.h>

I am so happy about this :)

> Source/WebCore/cssjit/RegisterAllocator.h:116
> +    Vector<JSC::MacroAssembler::RegisterID, calleeSavedRegisterCount> calleeSavedRegistersToReserve(unsigned count)

I think the name calleeSavedRegistersToReserve masks the side effects of this function.
What about: "preAllocateCalleeSavedRegisters(count)", or even keep "reserveCalleeSavedRegisters()"?

> Source/WebCore/cssjit/RegisterAllocator.h:118
> +        Vector<JSC::MacroAssembler::RegisterID, calleeSavedRegisterCount> registers;

Is this used?

> Source/WebCore/cssjit/RegisterAllocator.h:120
> +        ASSERT(!m_reservedCalleeSavedRegisters.size());

We can afford RELEASE_ASSERT here.

> Source/WebCore/cssjit/RegisterAllocator.h:129
> +    Vector<JSC::MacroAssembler::RegisterID, calleeSavedRegisterCount> calleeSavedRegistersToRestore()

restoreCalleeSavedRegisters/returnPreAllocatedCalleeSavedRegisters?

> Source/WebCore/cssjit/SelectorCompiler.cpp:550
> -#if CSS_SELECTOR_JIT_DEBUGGING && ASSERT_DISABLED
> +#if CSS_SELECTOR_JIT_DEBUGGING

IIRC, this was for certain debug builds, I can't remember why, that was during the initial proof-of-concept work.

Did you try debug builds?

> Source/WebCore/cssjit/SelectorCompiler.cpp:865
> +    

Spaaaaaaces

> Source/WebCore/cssjit/SelectorCompiler.cpp:871
> +static inline bool willGenerateFunctionCalls(const SelectorFragmentList& selectorFragments)
> +{
> +    // FIXME: If this selector doesn't call any functions, then we don't need to push the link register or frame pointer.
> +    UNUSED_PARAM(selectorFragments);
> +    return true;
> +}

I would add this with the patch, not ahead of time.

> Source/WebCore/cssjit/SelectorCompiler.cpp:872
> +    

Spaaaaaaces

> Source/WebCore/cssjit/SelectorCompiler.cpp:883
> +        prologueRegister.append(JSC::X86Registers::ebp);

You can use GPRInfo::callFrameRegister to have a name for it.

> Source/WebCore/cssjit/SelectorCompiler.cpp:915
> +    bool reservedCalleeSavedRegisters = m_registerAllocator.availableRegisterCount() < minimumRegisterCountForAttributes;
> +    bool callsFunctions = willGenerateFunctionCalls(m_selectorFragments);
> +    generatePrologue(callsFunctions, reservedCalleeSavedRegisters, minimumRegisterCountForAttributes);

I would just have:
    bool needsEpilogue = generatePrologue(callsFunctions, reservedCalleeSavedRegisters, minimumRegisterCountForAttributes)

> Source/WebCore/cssjit/SelectorCompiler.cpp:952
> +        if (!m_needsAdjacentBacktrackingStart && !reservedCalleeSavedRegisters && !callsFunctions) {

&& !needsEpilogue

> Source/WebCore/cssjit/SelectorCompiler.cpp:955
> +            // Epilogue not needed because it wouldn't write anything.

Once you move the name to the condition, you could remove the comments.

The comments could still be valuable but I would have it as a little explanation at the beginning of this branch.
Or...you could make this little block into a method generateFunctionEndWithoutEpilogue().

> Source/WebCore/cssjit/StackAllocator.h:71
> +    Vector<StackReference> push(Vector<JSC::MacroAssembler::RegisterID> registerIDs)

const Vector<JSC::MacroAssembler::RegisterID>&

> Source/WebCore/cssjit/StackAllocator.h:78
> +        for (unsigned i = 0; i < registerCount - 1; i += 2) {

I wonder how clang compile this :)

> Source/WebCore/cssjit/StackAllocator.h:82
> +            stackReferences.append(StackReference(m_offsetFromTop - stackUnitInBytes / 2));
> +            stackReferences.append(StackReference(m_offsetFromTop));

Is this right?

I would have thought stackReferences.append(StackReference(m_offsetFromTop)); should go before stackReferences.append(StackReference(m_offsetFromTop - stackUnitInBytes / 2));

Or maybe you should to push i + 1 before i?

> Source/WebCore/cssjit/StackAllocator.h:116
> +        ASSERT(stackReferences.size() == registerCount);

RELEASE_ASSERT, if we run into this, something horribly wrong has happened.

> Source/WebCore/cssjit/StackAllocator.h:126
> +            RELEASE_ASSERT(stackReferences[i - 1] == m_offsetFromTop && stackReferences[i - 2] == m_offsetFromTop - stackUnitInBytes / 2);

Let's split this into two release assert for clarity.

> Source/WebCore/cssjit/StackAllocator.h:128
> +            m_assembler.popPair(registerIDs[i - 2], registerIDs[i - 1]);

Don't forget to update this if you change the order of push above.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.