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

JIT: Fix new helper calls for some block copies involving promoted locals #86246

Merged
merged 5 commits into from
May 15, 2023

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented May 15, 2023

The change in #85620 was missing a check for the case where the
destination is not on stack but the source is. The backend still uses a
helper call for this case.

The field-by-field case would also usually involve helper calls since it
would normally need write barriers; however, the exception were for
byref fields, so Span/ReadOnlySpan were particularly affected.

This hopefully fixes #85987 and dotnet/perf-autofiling-issues#17605; I'll keep them open and recheck their perf graphs in a week or so to verify.

…cals

The change in dotnet#85620 was missing a check for the case where the
destination is not on stack but the source is. The backend still uses a
helper call for this case.

The field-by-field case would also usually involve helper calls since it
would normally need write barriers; however, the exception were for
byref fields, so Span<T>/ReadOnlySpan<T> were particularly affected.
@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 May 15, 2023
@ghost ghost assigned jakobbotsch May 15, 2023
@ghost
Copy link

ghost commented May 15, 2023

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

Issue Details

The change in #85620 was missing a check for the case where the
destination is not on stack but the source is. The backend still uses a
helper call for this case.

The field-by-field case would also usually involve helper calls since it
would normally need write barriers; however, the exception were for
byref fields, so Span/ReadOnlySpan were particularly affected.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 15, 2023

Ideally the backend should be fixed to be smarter about these block copies. The first problem is that we call a write barrier helper for TYP_BYREF fields, which is incorrect and inefficient (#80086). Even with that fixed we still need to ensure atomic field copies (for TYP_BYREF pointers), but genCodeForCpObj uses movsq to implement that, which is very inefficient (#7469). I left some measurements in #85987 (comment).

Diffs of this change

win-x64 diffs of this change against parent of #85620

Diffs above do not include benchmarks collections as the superpmi-collect is broken for them currently.

@jakobbotsch jakobbotsch marked this pull request as ready for review May 15, 2023 14:56
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

@jakobbotsch jakobbotsch requested a review from EgorBo May 15, 2023 14:56
@EgorBo
Copy link
Member

EgorBo commented May 15, 2023

@jakobbotsch I see a few size regressions where we emit now CORINFO_HELP_CHECKED_ASSIGN_REF instead of CORINFO_HELP_ASSIGN_BYREF, is that expected?

@jakobbotsch
Copy link
Member Author

@jakobbotsch I see a few size regressions where we emit now CORINFO_HELP_CHECKED_ASSIGN_REF instead of CORINFO_HELP_ASSIGN_BYREF, is that expected?

Yes it's expected, those diffs are essentially just the reverse of #85620... using the checked write barrier helper that also increments dst/src instead of the "normal" checked write barrier.

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.

[Perf] Windows/x64: 4 Regressions on 5/2/2023 10:35:24 AM
2 participants