Skip to content

JIT: Bulk copy of byrefs #101761

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.

Sign up for GitHub

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

Jump to bottom
Merged
EgorBo merged 5 commits into from
May 3, 2024
Merged

JIT: Bulk copy of byrefs #101761

EgorBo merged 5 commits into from
May 3, 2024

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented May 1, 2024 •
edited
Loading

Closes #101699
Closes #8627

Use a bulk copy when we need to copy a struct with gc fields (non-null) instead of individual gc write barriers

using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Running; BenchmarkSwitcher.FromAssembly(typeof(MyBench).Assembly).Run(args); public class MyBench { GcStruct4 s4 = new GcStruct4( 100001.ToString(), 100002.ToString(), 100003.ToString(), 100004.ToString()); GcStruct9 s9 = default; [Benchmark] public object BoxS4() => s4; [Benchmark] public object BoxS9() => s9; } record struct GcStruct4( string a1, string a2, string a3, string a4); record struct GcStruct9( string a1, string a2, string a3, string a4, string a5, string a6, string a7, string a8, string a9);
Method Toolchain Mean Ratio
BoxS4 \runtime-main...\corerun.exe 8.160 ns 1.00
BoxS4 \runtime-pr...\corerun.exe 5.254 ns 0.64
BoxS9 \runtime-main...\corerun.exe 13.546 ns 1.00
BoxS9 \runtime-pr...\corerun.exe 6.136 ns 0.45
@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2024

@MihuBot
@MihuBot MihuBot mentioned this pull request May 1, 2024
Open
@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2024

@MihuBot
@MihuBot MihuBot mentioned this pull request May 1, 2024
Open
This was referenced May 1, 2024
Open
Closed
Open
@EgorBo EgorBo force-pushed the bulk-assignbyref branch from 97b1d70 to efa0292 Compare May 1, 2024 23:02
@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2024

@MihuBot
@MihuBot MihuBot mentioned this pull request May 1, 2024
Open
@EgorBo EgorBo marked this pull request as ready for review May 1, 2024 23:45
@EgorBo EgorBo requested a review from MichalStrehovsky as a code owner May 1, 2024 23:45
This was referenced May 2, 2024
Open
Closed
@dotnet dotnet deleted a comment from EgorBot May 2, 2024
@EgorBo
Copy link
Member Author

EgorBo commented May 2, 2024

@jkotas PTAL
@EgorBo EgorBo requested a review from jkotas May 2, 2024 01:46
jkotas
jkotas reviewed May 2, 2024
View reviewed changes
MichalStrehovsky
MichalStrehovsky reviewed May 2, 2024
View reviewed changes
@github-actions github-actions bot mentioned this pull request May 2, 2024
Closed
jkotas
jkotas approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks
@EgorBo
Copy link
Member Author

EgorBo commented May 2, 2024 •
edited
Loading

@jakobbotsch cc @dotnet/jit-contrib PTAL jit part, TryLowerBlockStoreAsGcBulkCopyCall method is based on the tricks we used in LowerBlockStoreAsHelperCall (morphing calls in lower).
@EgorBo EgorBo requested a review from jakobbotsch May 2, 2024 11:00
@build-analysis build-analysis bot mentioned this pull request May 2, 2024
Closed
This was referenced May 2, 2024
Closed
Closed
jakobbotsch
jakobbotsch reviewed May 3, 2024
View reviewed changes
jakobbotsch
jakobbotsch reviewed May 3, 2024
View reviewed changes
src/coreclr/jit/lower.cpp
Comment on lines 8239 to 8244
if (data->OperIs(GT_IND))
{
// Drop GT_IND nodes
BlockRange().Remove(data);
data = data->AsIndir()->Addr();
}
Copy link
Member

@jakobbotsch jakobbotsch May 3, 2024

Choose a reason for hiding this comment

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

Do we need to worry about any indir flags here, like GTF_IND_VOLATILE?
Copy link
Member Author

@EgorBo EgorBo May 3, 2024

Choose a reason for hiding this comment

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

Good question, I think it's safer to just give up on those here, fixed.
@dotnet dotnet deleted a comment from EgorBot May 3, 2024
@EgorBo
Copy link
Member Author

EgorBo commented May 3, 2024

Failures are known - #101721, #101618
@EgorBo EgorBo merged commit 7ac5ef9 into dotnet:main May 3, 2024
@EgorBo EgorBo deleted the bulk-assignbyref branch May 3, 2024 15:54
This was referenced May 3, 2024
Closed
Closed
@dotnet dotnet deleted a comment from EgorBot May 3, 2024
@EgorBo
Copy link
Member Author

EgorBo commented May 7, 2024 •
edited
Loading

Improvements on x64:
@DrewScoggins DrewScoggins mentioned this pull request May 9, 2024
Closed
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
@EgorBo @michaelgsharp
JIT: Bulk copy of byrefs (dotnet#101761)
278081f
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@EgorBo @Ruihan-Yin
JIT: Bulk copy of byrefs (dotnet#101761)
bfd5430
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-crossgen2-coreclr

4 participants

@EgorBo @jkotas @jakobbotsch @MichalStrehovsky