Bug 125771 - Add a simple register allocator to WebCore for x86_64
Summary: Add a simple register allocator to WebCore for x86_64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-16 01:40 PST by Benjamin Poulain
Modified: 2013-12-17 02:36 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.92 KB, patch)
2013-12-16 01:47 PST, Benjamin Poulain
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2013-12-16 01:40:46 PST
Add a simple register allocator to WebCore for x86_64
Comment 1 Benjamin Poulain 2013-12-16 01:47:15 PST
Created attachment 219303 [details]
Patch
Comment 2 Geoffrey Garen 2013-12-16 15:52:50 PST
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.
Comment 3 Benjamin Poulain 2013-12-17 02:36:26 PST
Committed r160697: <http://trac.webkit.org/changeset/160697>