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

Use ClassLayout::AreCompatible in fgMorphCanUseLclFldForCopy #69642

Conversation

jakobbotsch
Copy link
Member

I was looking at removing the quirk in impPopCallArgs and tracked the positive diffs down to this.

All credit goes to @SingleAccretion for being able to make this change.

@ghost ghost assigned jakobbotsch May 21, 2022
@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 21, 2022
@ghost
Copy link

ghost commented May 21, 2022

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

Issue Details

I was looking at removing the quirk in impPopCallArgs and tracked the positive diffs down to this.

All credit goes to @SingleAccretion for being able to make this change.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

I believe the main benefit here is when we have instantiated generic structs that differ in exact types and __Canon (this happens frequently due to inlining).

Comment on lines -9659 to +9655
// TODO-PhysicalVN: with the physical VN scheme, this method is no longer needed.
// True if the second local is valid and has a layout compatible with the first local.
Copy link
Contributor

@SingleAccretion SingleAccretion May 21, 2022

Choose a reason for hiding this comment

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

The TODO should stay or the whole method deleted (what #68986 does).

There is no reason we cannot use LCL_FLD<lcl> in a construct such as the following:

lcl.StructField = promotedLocal;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I have not noticed that #68986 removes this, I will just leave it for that PR then.

There is no reason we cannot use LCL_FLD in a construct such as the following:

I see, makes sense. My main worry here was about dropping GC ness during the copy, but I guess the alternatives do not do anything different in that regard.

@jakobbotsch jakobbotsch deleted the use-class-layout-in-field-by-field-copy branch May 21, 2022 17:21
@ghost ghost locked as resolved and limited conversation to collaborators Jun 20, 2022
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