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/Windows/Arm64: Add TLS inline support #104282

Merged
merged 13 commits into from
Jul 3, 2024
Merged

NativeAOT/Windows/Arm64: Add TLS inline support #104282

merged 13 commits into from
Jul 3, 2024

Conversation

kunalspathak
Copy link
Member

The relocation for TLS variable is done by emitting a pair of add instructions in the following format:

add         x1,x1,tls_InlinedThreadStatics,lsl #0xC
add         x1,x1,tls_InlinedThreadStatics

The relocations are recorded as follows:

 00000030  SECREL_HIGH12A             91400021        62  tls_InlinedThreadStatics
 00000034  SECREL_LOW12L              91000021        62  tls_InlinedThreadStatics

The final disassembly looks like:

00007FF6B7AFA6E4  ldr         x0,[xpr,#0x58]  
00007FF6B7AFA6E8  adrp        x1,WKS::gc_heap::generation_table+420h (07FF6B7BFD000h)  
00007FF6B7AFA6EC  add         x1,x1,#0x2CC  
00007FF6B7AFA6F0  ldr         w1,[x1]  
00007FF6B7AFA6F4  ldr         x0,[x0,x1 lsl #3]  
00007FF6B7AFA6F8  add         x1,x1,#0,lsl #0xC  
00007FF6B7AFA6FC  add         x1,x1,#4

Contributes to #79521

@kunalspathak
working with 1 change remaining
12fa9bc 
hack to test

some fixes

almost trhere

fix assert

Some comments and revert sample Program

some renaming

Revert changes for IF_DI_1E

format IF_DI_2A code:

Format emitIns_Add_Add_Tls_Reloc

Fixed the code in IF_DI_2A
@kunalspathak
revert changes in reproNative.vcxproj
a1f1738
@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 Jul 2, 2024
@kunalspathak kunalspathak mentioned this pull request Jul 2, 2024
Closed
@kunalspathak kunalspathak requested a review from jkotas July 2, 2024 04:12
@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.

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@kunalspathak kunalspathak changed the title Windows/Arm64: Adds TLS inline support NativeAOT/Windows/Arm64: Add TLS inline support Jul 2, 2024
@kunalspathak
Copy link
Member Author

I forgot to update the guid. Will do it once i see some superpmi diff runs for JIT arm64, because I changed a code in lowering.

@kunalspathak kunalspathak requested a review from VSadov July 2, 2024 15:37
jakobbotsch
jakobbotsch approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

JIT side LGTM with some nits.

@kunalspathak
Copy link
Member Author

@jkotas @VSadov - this is ready again. Let me know if you have more feedback.

VSadov
VSadov reviewed Jul 2, 2024
View reviewed changes
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
@@ -3909,6 +3909,8 @@ private static RelocType GetRelocType(TargetArchitecture targetArchitecture, ush
const ushort IMAGE_REL_ARM64_BRANCH26 = 3;
const ushort IMAGE_REL_ARM64_PAGEBASE_REL21 = 4;
const ushort IMAGE_REL_ARM64_PAGEOFFSET_12A = 6;
const ushort IMAGE_REL_ARM64_SECREL_HIGH12A = 0xA;
const ushort IMAGE_REL_ARM64_SECREL_LOW12L = 0xB;
Copy link
Member

@VSadov VSadov Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: some constants here match values from RelocType and some match the actual platform-specific values (for the ease of disassembling?).

If that is a case, then maybe always use platform-specific values?
I.E. IMAGE_REL_ARM64_TLSDESC_CALL would match R_AARCH64_TLSDESC_CALL, which is 0x239

Or the other way - just use RelocType values for all?

Copy link
Member

Choose a reason for hiding this comment

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

Does not need to be addressed in this change since the inconsistency is already there.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, probably will not include in this PR.

@kunalspathak @jkotas
Update src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocatio…
565f2d7 
…n.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@build-analysis build-analysis bot mentioned this pull request Jul 3, 2024
Closed
VSadov
VSadov approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks!!

@kunalspathak kunalspathak merged commit f92499d into dotnet:main Jul 3, 2024
91 of 93 checks passed
@kunalspathak kunalspathak deleted the tls-win-arm64 branch July 3, 2024 05:03
This was referenced Jul 4, 2024
Closed
Merged
@jkotas jkotas mentioned this pull request Jul 6, 2024
Closed
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Jul 9, 2024
Closed
@kunalspathak kunalspathak mentioned this pull request Jul 26, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@VSadov VSadov VSadov approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees

@kunalspathak kunalspathak

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kunalspathak @jkotas @jakobbotsch @VSadov