[webkit-reviews] review granted: [Bug 125771] Add a simple register allocator to WebCore for x86_64 : [Attachment 219303] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 16 15:54:42 PST 2013


Geoffrey Garen <ggaren at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 125771: Add a simple register allocator to WebCore for x86_64
https://bugs.webkit.org/show_bug.cgi?id=125771

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.


More information about the webkit-reviews mailing list