Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] B3/Air should use the system register convention #8005

Conversation

jjgriego
Copy link
Contributor

@jjgriego jjgriego commented Dec 22, 2022

cbff45e

[JSC] B3/Air should use the system register convention
https://bugs.webkit.org/show_bug.cgi?id=249776

Reviewed by Yusuke Suzuki.

A previous patch (189dce5799e8) changed several of these locations to use the
`vmCalleeSaveRegisters` over the `calleeSaveRegisters`--change them back.

Though on most platforms with Air support, these two register sets are
identical, on ARMv7, which is new to the Air club, fewer registers are included
in the VM register calee-save set than in the system's callee-save set, which
caused problems with unwinding.

This patch also addresses the ARMv7-only issue with unwinding: if an Air callee
in a VM stack being unwound saves registers other than VM callee-saves, they
simply won't be restored--this is OK for the time being as VM callers will only
expect the VM callee-saves to be preserved. Eventually we may want to enable
code generation on ARMv7 to select the VM convention over the system one to
avoid unnecessary save/restores in ARM-generated code. (The alternative, making
vmCalleeSaveRegisters use all of the available platform callee-saves, isn't
appealing, as we want more temporary registers available to the assembler and
lower tiers)

Note that the usage in `B3StackGenerationParams.cpp` was actually more-or-less
original to that file. Still, it seems correct to use `calleeSaveRegisters`
there. (And again, on platforms other than ARMv7, the change is a no-op; the
sets are identical)

* Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp:
(JSC::B3::StackmapGenerationParams::unavailableRegisters const):
* Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:
(JSC::B3::Air::GenerateAndAllocateRegisters::prepareForGeneration):
* Source/JavaScriptCore/b3/air/AirCode.cpp:
(JSC::B3::Air::Code::Code):
* Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:
(JSC::B3::Air::handleCalleeSaves):
* Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp:
(JSC::B3::Air::lowerAfterRegAlloc):
* Source/JavaScriptCore/b3/air/testair.cpp:
* Source/JavaScriptCore/interpreter/Interpreter.cpp:
(JSC::UnwindFunctor::copyCalleeSavesToEntryFrameCalleeSavesBuffer const):

Canonical link: https://commits.webkit.org/258261@main

e93f2b8

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 ❌ πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ›  jsc-mips
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-mips-tests

@jjgriego jjgriego requested a review from a team as a code owner December 22, 2022 16:18
@jjgriego jjgriego self-assigned this Dec 22, 2022
@jjgriego jjgriego added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Dec 22, 2022
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@jjgriego jjgriego added the request-merge-queue Request a pull request to be added to merge-queue once ready label Dec 22, 2022
@jjgriego
Copy link
Contributor Author

I still don't have a commit bit--waiting on someone at Apple to countersign, as I understand it

@Constellation Constellation added merge-queue Applied to send a pull request to merge-queue and removed request-merge-queue Request a pull request to be added to merge-queue once ready labels Dec 22, 2022
https://bugs.webkit.org/show_bug.cgi?id=249776

Reviewed by Yusuke Suzuki.

A previous patch (189dce5) changed several of these locations to use the
`vmCalleeSaveRegisters` over the `calleeSaveRegisters`--change them back.

Though on most platforms with Air support, these two register sets are
identical, on ARMv7, which is new to the Air club, fewer registers are included
in the VM register calee-save set than in the system's callee-save set, which
caused problems with unwinding.

This patch also addresses the ARMv7-only issue with unwinding: if an Air callee
in a VM stack being unwound saves registers other than VM callee-saves, they
simply won't be restored--this is OK for the time being as VM callers will only
expect the VM callee-saves to be preserved. Eventually we may want to enable
code generation on ARMv7 to select the VM convention over the system one to
avoid unnecessary save/restores in ARM-generated code. (The alternative, making
vmCalleeSaveRegisters use all of the available platform callee-saves, isn't
appealing, as we want more temporary registers available to the assembler and
lower tiers)

Note that the usage in `B3StackGenerationParams.cpp` was actually more-or-less
original to that file. Still, it seems correct to use `calleeSaveRegisters`
there. (And again, on platforms other than ARMv7, the change is a no-op; the
sets are identical)

* Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp:
(JSC::B3::StackmapGenerationParams::unavailableRegisters const):
* Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:
(JSC::B3::Air::GenerateAndAllocateRegisters::prepareForGeneration):
* Source/JavaScriptCore/b3/air/AirCode.cpp:
(JSC::B3::Air::Code::Code):
* Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:
(JSC::B3::Air::handleCalleeSaves):
* Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp:
(JSC::B3::Air::lowerAfterRegAlloc):
* Source/JavaScriptCore/b3/air/testair.cpp:
* Source/JavaScriptCore/interpreter/Interpreter.cpp:
(JSC::UnwindFunctor::copyCalleeSavesToEntryFrameCalleeSavesBuffer const):

Canonical link: https://commits.webkit.org/258261@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/b3-air-should-use-system-register-convention branch from e93f2b8 to cbff45e Compare December 22, 2022 22:34
@webkit-early-warning-system webkit-early-warning-system merged commit cbff45e into WebKit:main Dec 22, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 258261@main (cbff45e): https://commits.webkit.org/258261@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
4 participants