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

Perform ldr to ldp peephole optimization #84399

Merged
merged 5 commits into from
Apr 8, 2023
Merged

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Apr 6, 2023

In #77540 we started replacing consecutive loads and stores with ldp and stp except if they are for local variables. See #77540 (review) for details. For loads, turns out that we do not need offset tracking for loads and gc-liveness tracking for ldp is already present. So this PR just relaxes the rule and enable the optimization for 2 consecutive loads.

For non-gc stores too, it enabled the peephole optimization because those variables do not need gc liveness tracking.

Contributes to #81278 and #55365
Fixes: #35132 and #35130

@ghost ghost assigned kunalspathak Apr 6, 2023
@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 Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

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

Issue Details

In local asmdiff verification, I see that the gcrefs are correctly tracked. Just double check if there are any GC holes.

Contributes to #81278

Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

gcstress run is clean.

@kunalspathak kunalspathak marked this pull request as ready for review April 6, 2023 19:56
@kunalspathak
Copy link
Member Author

Diffs from earlier run.

image

@BruceForstall
Copy link
Member

Diffs

Diffs look great.

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.

You didn't update the comment above: "Don't do this if either instruction had a local variable." It would be useful to update this and provide more detail about why ldr is allowed.

It also occurs to me that you could also allow str of locals if both str are non-GC types, e.g., floating-point/SIMD, or even TYP_INT. I still think in both ldr and str cases it would be helpful in DEBUG to be able to annotate JitDisasm with the local var (+offset) being read/written, for each local.

@kunalspathak
Copy link
Member Author

You didn't update the comment above: "Don't do this if either instruction had a local variable." It would be useful to update this and provide more detail about why ldr is allowed.

Done

It also occurs to me that you could also allow str of locals if both str are non-GC types, e.g., floating-point/SIMD, or even TYP_INT.

That's true. Added it.

I still think in both ldr and str cases it would be helpful in DEBUG to be able to annotate JitDisasm with the local var (+offset) being read/written, for each local.

Can we do it as a follow-up PR? I will open a .NET 8 tracking issue for it.

@BruceForstall
Copy link
Member

Can we do it as a follow-up PR?

yes. And please open an issue.

@kunalspathak
Copy link
Member Author

yes. And please open an issue.

#84504

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

gc-stress failures are unrelated.

@kunalspathak kunalspathak merged commit 0fc78e6 into dotnet:main Apr 8, 2023
@kunalspathak kunalspathak deleted the ldp branch April 8, 2023 21:16
@ghost ghost locked as resolved and limited conversation to collaborators May 9, 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.

ARM64: Optimize pair of "ldr reg, [reg]" to ldp
2 participants