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

ARM64: Use stlur for volatile stores #91553

Merged
merged 8 commits into from
Sep 8, 2023
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Sep 4, 2023

volatile int x;

void Test() => 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

@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 Sep 4, 2023
@ghost ghost assigned EgorBo Sep 4, 2023
@ghost
Copy link

ghost commented Sep 4, 2023

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

Issue Details
volatile int x;

void Test() => 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
Member Author

EgorBo commented Sep 5, 2023

@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
Member Author

EgorBo commented Sep 5, 2023

also, cc @VSadov

src/coreclr/jit/codegenarm64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenarm64.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member

Will this need a fix similar to #91668?

@EgorBo
Copy link
Member Author

EgorBo commented Sep 7, 2023

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
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
Member Author

EgorBo commented Sep 7, 2023

@BruceForstall PTAL refactored changes

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM. One question/suggestion.

src/coreclr/jit/codegenarmarch.cpp Outdated Show resolved Hide resolved
@EgorBo EgorBo merged commit f3c7893 into dotnet:main Sep 8, 2023
@EgorBo EgorBo deleted the stlur-volatile branch September 8, 2023 09:38
@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR 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