Add a simple register allocator to WebCore for x86_64

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Add a simple register allocator to WebCore for x86_64

[Benjamin Poulain]

Created attachment 219303 [details]
Patch

[Geoffrey Garen]

Comment on attachment 219303 [details]
Patch

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

r=me

> Source/WebCore/cssjit/RegisterAllocator.h:51
> +    void reserveRegister(JSC::MacroAssembler::RegisterID registerID)

I think you could call this "allocateRegister". It fits nicely in the C++ function overloading model: The extra argument explains the difference in behavior from the version of allocateRegister that doesn't take an argument.

> Source/WebCore/cssjit/RegisterAllocator.h:63
> +    void returnRegister(JSC::MacroAssembler::RegisterID registerID)

I would call this "deallocateRegister", to match more closely with "allocateRegister".

> Source/WebCore/cssjit/RegisterAllocator.h:66
> +        m_allocatedRegisters.remove(m_allocatedRegisters.reverseFind(registerID));

I think you should add a comment here explaining that we use reverseFind because we almost always return registers in a stack-like order.

> Source/WebCore/cssjit/RegisterAllocator.h:110
> +    m_registers.append(JSC::X86Registers::eax);
> +    m_registers.append(JSC::X86Registers::ecx);
> +    m_registers.append(JSC::X86Registers::esi);
> +    m_registers.append(JSC::X86Registers::edi);
> +    m_registers.append(JSC::X86Registers::r8);
> +    m_registers.append(JSC::X86Registers::r9);
> +    m_registers.append(JSC::X86Registers::r10);
> +    m_registers.append(JSC::X86Registers::r11);

I think this would be clearer if:

(a) The list of registers were in a static const array, and you looped over the array, calling append;

and

(b) You added a comment explaining that these are the caller-save registers, and we use them because we want to avoid saving registers at the head of our JITed function.

[Benjamin Poulain]

Committed r160697: <http://trac.webkit.org/changeset/160697>