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

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied 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 434835: Patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 434835
  --> https://bugs.webkit.org/attachment.cgi?id=434835
Patch

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

> Source/JavaScriptCore/b3/air/AirCode.cpp:96
> +#if !CPU(ARM64)
>      if (auto reg = pinnedExtendedOffsetAddrRegister())
>	   pinRegister(*reg);
> +#endif

I wouldn't write this patch this way.
- This call is now a no-op, since no other platform provided a value for
pinnedExtendedOffsetAddrRegister
- I think I'd just directly use the linkRegister in those places that use
`pinnedExtendedOffsetAddrRegister`.
- pinnedExtendedOffsetAddrRegister is now a wrong name, since it's no longer
pinned.

I think the cleanest patch is to remove pinnedExtendedOffsetAddrRegister and
just use linkRegister inside those phases. You could write a helper function
that just returns the linkRegister on arm64, and crashes on other platforms, so
you can keep various code runtime enabled. But you could also just change the
code to using #if CPU(ARM64) instead of "if (isARM64())"