Bug 133156 – make css jit work on arm64

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

[Alex Christensen]

The css jit works on arm64.  It still needs to be tested and optimized so I won't enable it yet.

[Alex Christensen]

Created attachment 231836 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231836 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:879
> +    generatePrologue();

Why doesn't x86_64 need a prologue? Doesn't it need to save the frame pointer register (%rbp)?

[Alex Christensen]

(In reply to comment #2)
> Why doesn't x86_64 need a prologue? Doesn't it need to save the frame pointer register (%rbp)?
Doesn't x86_64 do that with the CALL instruction?  This will need to be optimized later to only push and pop the link register and frame pointer if it calls a function, but this is just to get it working so we can optimize from here.

[Benjamin Poulain]

Comment on attachment 231836 [details]
Patch

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

> Source/WebCore/cssjit/SelectorCompiler.cpp:874
> +#if CPU(ARM64)
> +    m_assembler.pushPair(JSC::ARM64Registers::lr, JSC::ARM64Registers::fp);
> +#endif
> +}
> +
> +void SelectorCodeGenerator::generateEpilogue()
> +{
> +#if CPU(ARM64)
> +    m_assembler.popPair(JSC::ARM64Registers::lr, JSC::ARM64Registers::fp);
> +#endif

I would prefer this to be done through the stack allocator.

Here, if we have a mismatch of pushPair/popPair, we could run into security problems.

> Source/WebCore/cssjit/SelectorCompiler.cpp:935
> +            generateEpilogue();
>              m_assembler.ret();
>  
>              // Failure.
>              if (!failureCases.empty()) {
>                  failureCases.link(&m_assembler);
>                  m_assembler.move(Assembler::TrustedImm32(0), returnRegister);
> +                generateEpilogue();
>                  m_assembler.ret();

The duplicated epilogue may be slower than an unconditional jump. You may want to generalize reservedCalleeSavedRegisters.

[Alex Christensen]

Created attachment 231910 [details]
Patch

[Alex Christensen]

Created attachment 231917 [details]
Patch

[Alex Christensen]

Created attachment 231918 [details]
Patch

[Benjamin Poulain]

Comment on attachment 231918 [details]
Patch

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.

[Yusuke Suzuki]

Comment on attachment 231918 [details]
Patch

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

Great!

> Source/WebCore/cssjit/StackAllocator.h:105
>          m_assembler.push(registerID);

Since MacroAssembler provides `pushToSave` for all environments, we can remove this ifdef and simply use `pushToSave` for x86_64 environment :)

> Source/WebCore/cssjit/StackAllocator.h:146
> +        m_assembler.popToRestore(registerID);

Likewise we can use `popToRestore` for `StackAllocator::pop` and remove ifdef :)

[Alex Christensen]

(In reply to comment #8)
> > 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?
Yes.  It works fine.  I think this should be changed to make debugging debug builds easier.
> > Source/WebCore/cssjit/StackAllocator.h:82
> > +            stackReferences.append(StackReference(m_offsetFromTop - stackUnitInBytes / 2));
> > +            stackReferences.append(StackReference(m_offsetFromTop));
> 
> Is this right?
Yes.  The order is arbitrary, but it has to be consistent.  This way the registers are pushed in the order they are in the vector and popped in the opposite order of the given vector, which I think makes the most sense and is most like x86_64.

I fixed all other comments.

[Alex Christensen]

Created attachment 231973 [details]
Patch

[Benjamin Poulain]

(In reply to comment #10)
> > Did you try debug builds?
> Yes.  It works fine.  I think this should be changed to make debugging debug builds easier.
> > > Source/WebCore/cssjit/StackAllocator.h:82
> > > +            stackReferences.append(StackReference(m_offsetFromTop - stackUnitInBytes / 2));
> > > +            stackReferences.append(StackReference(m_offsetFromTop));
> > 
> > Is this right?
> Yes.  The order is arbitrary, but it has to be consistent.  This way the registers are pushed in the order they are in the vector and popped in the opposite order of the given vector, which I think makes the most sense and is most like x86_64.

The order in which you store registers on the stack can be arbitrary. But the StackReferences are used to access values by address, they need to match the order in which the register are stored on the stack.

[Alex Christensen]

(In reply to comment #12)
> The order in which you store registers on the stack can be arbitrary. But the StackReferences are used to access values by address, they need to match the order in which the register are stored on the stack.
They do.

[Alex Christensen]

Created attachment 231975 [details]
Patch

[Alex Christensen]

Created attachment 231989 [details]
Patch

[Benjamin Poulain]

Comment on attachment 231989 [details]
Patch

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

> Source/WebCore/cssjit/RegisterAllocator.h:125
> +        return Vector<JSC::MacroAssembler::RegisterID, calleeSavedRegisterCount>(m_reservedCalleeSavedRegisters);

Do you need to explicit copy constructor here?

> Source/WebCore/cssjit/StackAllocator.h:76
> +        stackReferences.reserveCapacity(registerCount);

reserveInitialCapacity()

> Source/WebCore/cssjit/StackAllocator.h:113
> +        ASSERT(m_offsetFromTop >= stackUnitInBytes * registerCount);

Is this right on ARM64? You could have 2 registers by stack units.

This is probably only good for x86_64.

[Alex Christensen]

http://trac.webkit.org/changeset/169297