Bug 228710 – [ARM64] Use link register instead of pinning a register for materializing big load constants

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Yijia Huang]

Created attachment 434782 [details]
Patch

[Yijia Huang]

Created attachment 434835 [details]
Patch

[Saam Barati]

Comment on attachment 434835 [details]
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())"

[Yijia Huang]

Created attachment 434843 [details]
Patch

[Yijia Huang]

Created attachment 434845 [details]
Patch

[Mark Lam]

Comment on attachment 434845 [details]
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()"?

[Yijia Huang]

Created attachment 434849 [details]
Patch for landing

[Yijia Huang]

Created attachment 434851 [details]
Patch for landing

[Mark Lam]

Comment on attachment 434851 [details]
Patch for landing

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

> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:81
> +                        ASSERT(linkRegister());

Can remove this because this code only applies to isARM64().

> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:131
> +                        ASSERT(linkRegister());

Ditto.  Can remove.

[Yijia Huang]

Created attachment 434856 [details]
Patch for landing

[EWS]

Committed r280609 (240227@main): <https://commits.webkit.org/240227@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434856 [details].

[Radar WebKit Bug Importer]

<rdar://problem/81477534>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 228791

[Yijia Huang]

Created attachment 434988 [details]
Patch for landing

[Yijia Huang]

Created attachment 434993 [details]
Patch for reviewing

[Saam Barati]

Comment on attachment 434993 [details]
Patch for reviewing

Why are we pinning the data temp register? Isn’t the point of this patch to stop doing that?

[Yijia Huang]

The modifications to replace the pinned register with the LR should be simple. However, when we try to remove this in AirCode.cpp:

```
if (auto reg = pinnedExtendedOffsetAddrRegister())
     pinRegister(*reg);
```

Some stress tests fail. And one of them is:

```
stress/dont-assert-ai-proved-array-mode.js.ftl-no-cjit-b3o0: test_script_4413: line 2: 24736 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Helpers/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --validateExceptionChecks\=true --useDollarVM\=true --maxPerThreadStackUsage\=1572864 --useArrayAllocationProfiling\=false --forcePolyProto\=true --useRandomizingExecutableIslandAllocation\=true --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true --maxDFGNodesInBasicBlockForPreciseAnalysis\=100 --defaultB3OptLevel\=0 --forceOSRExitToLLInt\=true --jitPolicyScale\=0 --slowPathAllocsBetweenGCs\=15 dont-assert-ai-proved-array-mode.js )
stress/dont-assert-ai-proved-array-mode.js.ftl-no-cjit-b3o0: ERROR: Unexpected exit code: 139
```

[Yijia Huang]

Pull request: https://github.com/WebKit/WebKit/pull/2665

[EWS]

Committed 253105@main (fb402aa86c16): <https://commits.webkit.org/253105@main>

Reviewed commits have been landed. Closing PR #2665 and removing active labels.