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: Prefer block copies for unenregisterable locals even with GC pointers #85620

Merged
merged 2 commits into from
May 2, 2023

Conversation

jakobbotsch
Copy link
Member

This comment seems outdated/wrong, the backend's block copy strategy when GC pointers are involved is highly tuned and often no helper is necessary at all.

…nters

This comment seems outdated/wrong, the backend's block copy strategy
when GC pointers are involved is highly tuned and often no helper is
necessary at all.
@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 2, 2023
@ghost ghost assigned jakobbotsch May 2, 2023
@ghost
Copy link

ghost commented May 2, 2023

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

Issue Details

This comment seems outdated/wrong, the backend's block copy strategy when GC pointers are involved is highly tuned and often no helper is necessary at all.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch marked this pull request as ready for review May 2, 2023 09:20
@jakobbotsch
Copy link
Member Author

Opened #85637 for the failure.

cc @dotnet/jit-contrib PTAL @EgorBo. Diffs -- not bad for a simple change.

@jakobbotsch jakobbotsch requested a review from EgorBo May 2, 2023 09:20
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Nice! So we now do BLK copy where previously we used to do field-by-field copy/init due to gc pointers?

@jakobbotsch
Copy link
Member Author

Nice! So we now do BLK copy where previously we used to do field-by-field copy/init due to gc pointers?

Indeed (when we know the struct is going to be on stack anyway).

@EgorBo
Copy link
Member

EgorBo commented May 2, 2023

Although, I guess we'll still be doing GPR movs (can't use SIMD for structs with gc handles) so I thought it's basically the same as physical promotion?

@jakobbotsch
Copy link
Member Author

Although, I guess we'll still be doing GPR movs (can't use SIMD for structs with gc handles) so I thought it's basically the same as physical promotion?

The backend does use SIMD copies even when GC pointers are involved by disabling GC during the copy.

@jakobbotsch jakobbotsch merged commit dc796f7 into dotnet:main May 2, 2023
@jakobbotsch jakobbotsch deleted the block-morph-gc-pointers branch May 2, 2023 10:29
jakobbotsch added a commit to jakobbotsch/runtime that referenced this pull request May 15, 2023
…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.
jakobbotsch added a commit that referenced this pull request May 15, 2023
…cals (#86246)

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<T>/ReadOnlySpan<T> were particularly affected.
@ghost ghost locked as resolved and limited conversation to collaborators Jun 1, 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.

2 participants