[webkit-reviews] review denied: [Bug 133156] make css jit work on arm64 : [Attachment 231918] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 133156: make css jit work on arm64
https://bugs.webkit.org/show_bug.cgi?id=133156

Attachment 231918: Patch
https://bugs.webkit.org/attachment.cgi?id=231918&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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_registerAll
ocator.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.