[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