Bug 228710 - [ARM64] Use link register instead of pinning a register for materializing big load constants
Summary: [ARM64] Use link register instead of pinning a register for materializing big...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
Depends on: 228791
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-02 11:47 PDT by Yijia Huang
Modified: 2021-08-25 13:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.25 KB, patch)
2021-08-02 11:51 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (2.42 KB, patch)
2021-08-03 08:42 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2021-08-03 11:22 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (6.12 KB, patch)
2021-08-03 11:38 PDT, Yijia Huang
mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (5.95 KB, patch)
2021-08-03 12:13 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (5.91 KB, patch)
2021-08-03 12:25 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch for landing (5.81 KB, patch)
2021-08-03 12:53 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch for landing (5.83 KB, patch)
2021-08-05 08:39 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for reviewing (6.39 KB, patch)
2021-08-05 09:12 PDT, Yijia Huang
sbarati: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-08-02 11:47:13 PDT
...
Comment 1 Yijia Huang 2021-08-02 11:51:39 PDT
Created attachment 434782 [details]
Patch
Comment 2 Yijia Huang 2021-08-03 08:42:57 PDT
Created attachment 434835 [details]
Patch
Comment 3 Saam Barati 2021-08-03 10:49:50 PDT
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())"
Comment 4 Yijia Huang 2021-08-03 11:22:43 PDT
Created attachment 434843 [details]
Patch
Comment 5 Yijia Huang 2021-08-03 11:38:33 PDT
Created attachment 434845 [details]
Patch
Comment 6 Mark Lam 2021-08-03 11:54:01 PDT
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()"?
Comment 7 Yijia Huang 2021-08-03 12:13:56 PDT
Created attachment 434849 [details]
Patch for landing
Comment 8 Yijia Huang 2021-08-03 12:25:32 PDT
Created attachment 434851 [details]
Patch for landing
Comment 9 Mark Lam 2021-08-03 12:32:38 PDT
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.
Comment 10 Yijia Huang 2021-08-03 12:53:44 PDT
Created attachment 434856 [details]
Patch for landing
Comment 11 EWS 2021-08-03 13:27:02 PDT
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].
Comment 12 Radar WebKit Bug Importer 2021-08-03 13:28:22 PDT
<rdar://problem/81477534>
Comment 13 WebKit Commit Bot 2021-08-04 12:17:52 PDT
Re-opened since this is blocked by bug 228791
Comment 14 Yijia Huang 2021-08-05 08:39:36 PDT
Created attachment 434988 [details]
Patch for landing
Comment 15 Yijia Huang 2021-08-05 09:12:21 PDT
Created attachment 434993 [details]
Patch for reviewing
Comment 16 Saam Barati 2021-08-05 09:21:15 PDT
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?
Comment 17 Yijia Huang 2021-08-25 13:17:21 PDT
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
```