Skip to content

ARM64: Use stlur for volatile stores - #91553

Merged
EgorBo merged 8 commits into
dotnet:mainfrom
EgorBo:stlur-volatile
Sep 8, 2023
Merged

ARM64: Use stlur for volatile stores#91553
EgorBo merged 8 commits into
dotnet:mainfrom
EgorBo:stlur-volatile

Conversation

@EgorBo

@EgorBoEgorBo commented Sep 4, 2023

Copy link
Copy Markdown
Member
volatileintx;voidTest()=>x=42;

Codegen diff on arm64 v8.4+ (e.g. Apple M2):

; Assembly listing for method Program:Test()
stp fp, lr, [sp, #-0x10]!
mov fp, sp
mov w1, #42
- dmb ish- str w1, [x0, #0x08]+ stlur w1, [x0, #0x08]
ldp fp, lr, [sp], #0x10
ret lr

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 4, 2023
@ghostghost assigned EgorBoSep 4, 2023
@ghost

ghost commented Sep 4, 2023

Copy link
Copy Markdown

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

Issue Details
volatileintx;voidTest()=>x=42;

Was:

; Assembly listing for method Program:Test()
stp fp, lr, [sp, #-0x10]!
mov fp, sp
mov w1, #42
- dmb ish- str w1, [x0, #0x08]+ stlur w1, [x0, #0x08]
ldp fp, lr, [sp], #0x10
ret lr
Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo
EgorBo marked this pull request as ready for review September 5, 2023 10:27
@EgorBo

Copy link
Copy Markdown
MemberAuthor

@TIHan @dotnet/jit-contrib PTAL, it's a follow-up to #89681 but for stores.

TLDR: on arm64-v8.4 we can use stlur instead of strl or str+dmb because the former now supports IMM offsets. stlur, just like ldapur, has acquire-release semantics which matches our memory model expectations.

@EgorBo

Copy link
Copy Markdown
MemberAuthor

also, cc @VSadov

Comment threadsrc/coreclr/jit/codegenarm64.cpp Outdated
Comment threadsrc/coreclr/jit/codegenarm64.cpp Outdated
@jakobbotsch

Copy link
Copy Markdown
Member

Will this need a fix similar to #91668?

@EgorBo

Copy link
Copy Markdown
MemberAuthor

Will this need a fix similar to #91668?

I'd guess - yes? since it's the same ISA.

// stlur 1X011001000iiiii iiii00nnnnnttttt 9900 0000 [Xn imm(-256..+255)] ARMv8.4 RCPC2
// stlurb 00011001000iiiii iiii00nnnnnttttt 1900 0000 [Xn imm(-256..+255)] ARMv8.4 RCPC2
// stlurh 01011001000iiiii iiii00nnnnnttttt 5900 0000 [Xn imm(-256..+255)] ARMv8.4 RCPC2

doc: https://developer.arm.com/documentation/ddi0602/2023-06/Base-Instructions/STLUR--Store-Release-Register--unscaled--?lang=en

can you add them as well?

@jakobbotsch

Copy link
Copy Markdown
Member

doc: https://developer.arm.com/documentation/ddi0602/2023-06/Base-Instructions/STLUR--Store-Release-Register--unscaled--?lang=en

can you add them as well?

Done (but untested)

@EgorBo

Copy link
Copy Markdown
MemberAuthor

@BruceForstall PTAL refactored changes

@BruceForstallBruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. One question/suggestion.

Comment threadsrc/coreclr/jit/codegenarmarch.cpp Outdated
@EgorBo
EgorBo merged commit f3c7893 into dotnet:mainSep 8, 2023
@EgorBo
EgorBo deleted the stlur-volatile branch September 8, 2023 09:38
@ghostghost locked as resolved and limited conversation to collaborators Oct 8, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@EgorBo@jakobbotsch@BruceForstall