[webkit-reviews] review granted: [Bug 228710] Use link register instead of pinning a register for materializing big load constants : [Attachment 434845] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yijia Huang <yijia_huang at apple.com>'s
request for review:
Bug 228710: Use link register instead of pinning a register for materializing
big load constants
https://bugs.webkit.org/show_bug.cgi?id=228710

Attachment 434845: Patch

https://bugs.webkit.org/attachment.cgi?id=434845&action=review




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 434845
  --> https://bugs.webkit.org/attachment.cgi?id=434845
Patch

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

r+ with fixes.

> Source/JavaScriptCore/ChangeLog:3
> +	   Use link register instead of pinning a register for materializing
big load constants

I suggest prefixing the title with [ARM64] so that it is clear that this change
only applies to ARM64 ports.

> Source/JavaScriptCore/ChangeLog:10
> +	   A link register is a special-purpose register which holds the
address to return to when a function call 
> +	   completes. This is more efficient than the more traditional scheme
of storing return addresses on a 
> +	   machine stack. Previouly, we pin a register for materializing a
large constant that cannot fit in Load/Store

I think the first 2 sentences are not needed because the use of lr is a
commonly known thing for anyone programming for ARM64.	Also, what is claimed
in these 2 sentences is not relevant to this patch.

typo: /Previouly/Previously/
I also suggest "pin a register for" ==> "pin a register as a temp for"

> Source/JavaScriptCore/ChangeLog:11
> +	   imm form. This is not efficient since the allocator has one less
register to access. To solve this problem, we 

I suggest "the allocator" ==> "the register allocator", and "to access" ==> "to
allocate from".

> Source/JavaScriptCore/ChangeLog:12
> +	   should switch to the link register supported by ARM64.

I suggest "switch to the link register supported by ARM64" ==> "switch to using
the link register as the temp on ARM64".

> Source/JavaScriptCore/b3/B3Common.cpp:70
> +std::optional<GPRReg> getLinkRegister()

I think WebKit style is to not prefix getters with "get" (except for some
special cases).  Can you rename this to simply "linkRegister()"?