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.

Sign up for GitHub

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

Jump to bottom

[NativeAOT/ARM64] Generate frames compatible with Apple compact unwinding #111451

Merged
merged 3 commits into from
Jan 22, 2025
Merged

[NativeAOT/ARM64] Generate frames compatible with Apple compact unwinding #111451

merged 3 commits into from
Jan 22, 2025
+337 −93

Conversation

filipnavara
Copy link
Member

Re-introduces original commit 4536b96 (PR #107766) which was reverted with bbe2cb9 (PR #111405).

The additional commit fixes the issue with GC info in GC stress. It uses the same approach as ARM32 where lvaFrameAddress has an a new parameter to suppress the FP-n => SP+m optimization when we are calculating the offsets for GC info purposes.

@filipnavara
[NativeAOT/ARM64] Generate frames compatible with Apple compact unwin…
35d6c08 
…ding (dotnet#107766)

* JIT/ARM64: Add ability to generate frames compatible with Apple compact
unwinding format.

For NativeAOT/ARM64/Apple API do the following:
- Save callee registers in opposite order and in pairs.
- Prefer saving FP/LR on the top of the frame. Heuristics are used to
  avoid worse code quality outside of prolog/epilog due to addressing
  range limits of the ARM64 instruction set.
- Added optimization to lvaFrameAddress to rewrite FP-x references to
  SP+y when possible. This allows efficient addressing using positive
  indexes when FP points to the top of the frame. It mimics similar
  optimization on ARM32.

* ObjWriter: For Mach-O ARM64 try to convert the DWARF CFI unwinding codes
into compact unwinding code

* Disable lvaFrameAddress FP->SP optimization for OSR methods
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 15, 2025
@filipnavara filipnavara requested a review from EgorBo January 15, 2025 10:31
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 15, 2025
@filipnavara
Fix variable offsets used in emitGCvarLiveUpd by suppressing the
Loading
Loading status checks…
2859b22 
FP-n => SP+m (for m = frameSize - n) optimization.
@filipnavara filipnavara force-pushed the arm64-compact-unwind-4 branch from 023e299 to 2859b22 Compare January 15, 2025 10:32
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@EgorBo
Copy link
Member

EgorBo commented Jan 15, 2025

/azp list

@azure-pipelines Azure Pipelines

This comment was marked as resolved.

Sign in to view
@EgorBo
Copy link
Member

EgorBo commented Jan 15, 2025

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Jan 15, 2025

Outerloop failures seem unrelated

@filipnavara
Copy link
Member Author

The win-x86 failure is #109500. We should reopen the issue.

@EgorBo EgorBo mentioned this pull request Jan 16, 2025
Open
@jakobbotsch
Copy link
Member

Can we open a separate issue? It does not look like the same crash symptom to me (different platform, unhandled managed exception vs AV).

@filipnavara
Copy link
Member Author

Can we open a separate issue? It does not look like the same crash symptom to me (different platform, unhandled managed exception vs AV).

Platform is different, but the pipeline is not and neither is the test... Notably, the linked issue was already mixing both osx-arm64 and linux-arm.

Log from the PR:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
at Assert.AreEqual(Object actual, Object expected)
at Program.TestStaticFields()
at Program.RunAllTests()
at Program.Main()

Linked issue:

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at Assert.AreEqual(Object actual, Object expected)
   at Program.TestStaticFields()
   at Program.RunAllTests()
   at Program.Main()

@jakobbotsch
Copy link
Member

Ah nevermind, there is an unhandled exception in the log.

@filipnavara
Copy link
Member Author

Can we (re-)merge this? /cc @kunalspathak

@kunalspathak
Merge branch 'main' into arm64-compact-unwind-4

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
348e71e
kunalspathak
kunalspathak reviewed Jan 22, 2025
View reviewed changes
src/coreclr/jit/emitarm64.cpp
ofs2Dist = EA_SIZE_IN_BYTES(size);
#ifdef DEBUG
assert(FPbased == FPbased2);
if (FPbased)
{
assert(id->idReg3() == REG_FP);
Copy link
Member

Choose a reason for hiding this comment

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

i forgot to ask, but why this assert was removed?

Copy link
Member Author

@filipnavara filipnavara Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The optimization in lvaFrameAddress can rewrite FP-n expression to SP+m in the instruction encoding which would be one of the inputs here. Since here we call lvaFrameAddress again with an additional parameter that suppresses this optimization (to always get FP-relative offset for purpose of GC info offset for something that was FP-relative in the first place), we can can get a mismatch. This mismatch is expected.

(The general idea of the optimization is that in the instruction encoding we can substitute the two because they refer to the same memory location and positive offsets have wider range that can be expressed in the instruction encoding. For the GC info offsets we want to use the original offsets due to the way how GC info is encoded. This closely matches the same logic in ARM32.)

kunalspathak
kunalspathak approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak kunalspathak merged commit 9031940 into dotnet:main Jan 22, 2025
109 checks passed
@kunalspathak kunalspathak mentioned this pull request Jan 24, 2025
Closed
filipnavara added a commit to filipnavara/runtime that referenced this pull request Jan 24, 2025
@filipnavara
Revert "[NativeAOT/ARM64] Generate frames compatible with Apple compa…
073db22 
…ct unwinding (dotnet#111451)"

This reverts commit 9031940.
@matouskozak matouskozak mentioned this pull request Jan 28, 2025
Closed
@kunalspathak kunalspathak mentioned this pull request Feb 7, 2025
Merged
@MichalStrehovsky MichalStrehovsky mentioned this pull request Feb 10, 2025
Open
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EgorBo EgorBo

@kunalspathak kunalspathak

@MichalStrehovsky MichalStrehovsky

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@filipnavara @EgorBo @jakobbotsch @kunalspathak