Skip to content

Recognize "Span.Length" during physical promotion and set IsNeverNegative flag #113615

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

Merged
merged 2 commits into from
Mar 17, 2025

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 17, 2025

Closes #104573

@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 Mar 17, 2025
@EgorBo EgorBo marked this pull request as ready for review March 17, 2025 16:56
@Copilot Copilot AI review requested due to automatic review settings March 17, 2025 16:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@EgorBo
Copy link
Member Author

EgorBo commented Mar 17, 2025

@jakobbotsch cc @dotnet/jit-contrib should we merge it as is then? Diffs (seems to be overall PerfScore improvement)

The end goal is to force spans being promoted for this snippet (win-x64):

static void Test(Span<int> span, int len)
{
    for (int i = 0; i < len; i++)
        span[i] = 0;
}

while currently (thanks to the old promotion) they're not.

@jakobbotsch
Copy link
Member

I would obviously like to take this change right now, but I'm worried about regressions like
image

They happen because new promotion never considers it beneficial to promote structs that are only used "whole", while it turns out that can be beneficial because it can reduce the number of struct copies (instead of "memory -> stack, stack -> memory" the promotion turns things into "memory -> registers -> memory").
It's a known deficiency right now and also shows up very frequently when I look at the diffs from disabling old promotion entirely. The fix for that is going to be something that still chooses to promote some of these only-whole-uses structs, but of course getting this right is probably also not going to be super simple...

Ideally we would allow physical promotion to take over when old promotion decides to undo promotion, but that also seems like a bunch of work...

@EgorBo
Copy link
Member Author

EgorBo commented Mar 17, 2025

@jakobbotsch Ok, I trust your instincts, how about merging the IsNeverNegative fix only? (changed) Diffs

@jakobbotsch
Copy link
Member

That sounds good to me, can you fix the PR title?

@EgorBo EgorBo changed the title Forward implicit byrefs to the new promotion to handle Recognize "Span.Length" during physical promotion and set IsNeverNegative flag Mar 17, 2025
@EgorBo EgorBo merged commit 4ff64a0 into dotnet:main Mar 17, 2025
111 of 113 checks passed
@EgorBo EgorBo deleted the leave-implicit-byrefs-for-new-promotion branch March 17, 2025 23:56
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2025
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.

JIT: "Never-negative" handling is not implemented for Span length fields promoted by physical promotion
2 participants