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

Do not generate TYP_STRUCT LCL_FLD nodes #66251

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Mar 5, 2022

Today, the morpher can fold IND(struct ADDR(LCL)) into LCL_FLD TYP_STRUCT layout-less nodes, these end up being locations, always (well -- there is one case where that's not true, such a node can end up under a return of a small struct later to be retyped by lowering, but this is rare).

These will be a significant impediment to actual, proper TYP_STRUCT local field nodes, as it would mean handling the case, everywhere in code, where the layout would be missing, that we don't want to allow in the first place.

This change fixed that by introducing the folding that the creation of these location nodes achieves at the address level, where we can choose the type of the location node arbitrarily and freely.

Along the way, IsLocalAddrExpr was fixed to not forget to look at zero-offset sequences attached to ADDRs, to avoid regressions.

We are expecting some positive diffs here, from the IsLocalAddrExpr change (enabling more precise VNs) and the folding change itself (the old code path would, ever so rarely, miss out on some things).

We are also expecting one small regression due to call retyping of reg-sized IND struct(ADDR(LCL)) nodes losing the field sequence. I did not fix it because a) it was small, and rare, b) I do not want to permeate the zero-offset code any more than is strictly required.

Diffs.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 5, 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 Mar 5, 2022
@ghost
Copy link

ghost commented Mar 5, 2022

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

Issue Details

Today, the morpher can fold IND(struct ADDR(LCL)) into LCL_FLD TYP_STRUCT layout-less nodes, these end up being locations, always (well -- there is one case where that's not true, such a node can end up under a return of a small struct later to be retyped by lowering, but this is rare).

These will be a significant impediment to actual, proper TYP_STRUCT local field nodes, as it would mean handling the case, everywhere in code, where the layout would be missing, that we don't want to allow in the first place.

This change fixed that by introducing the folding that the creation of these location nodes achieves at the address level, where we can choose the type of the location node arbitrarily and freely.

Along the way, IsLocalAddrExpr was fixed to not forget to look at zero-offset sequences attached to ADDRs, to avoid regressions.

We are expecting some positive diffs here, from the IsLocalAddrExpr change (enabling more precise VNs) and the folding change itself (the old code path would, ever so rarely, miss out on some things).

We are also expecting one small regression due to call retyping of reg-sized IND struct(ADDR(LCL)) nodes losing the field sequence. I did not fix it because a) it was small, and rare, b) I do not want to permeate the zero-offset code any more than is strictly required.

Blocked on #66247.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion force-pushed the NoStruct-LclFld branch 3 times, most recently from f5a918a to a6cae9a Compare March 9, 2022 11:55
This is also a correctness fix. Missing adding zero-offset field sequences is fatal.
Types of location nodes do not matter, the underlying locations do.
@SingleAccretion SingleAccretion marked this pull request as ready for review March 25, 2022 19:06
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@kunalspathak kunalspathak self-requested a review March 28, 2022 16:57
@kunalspathak kunalspathak self-assigned this Mar 28, 2022
@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Mar 28, 2022
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Couple of comments.

src/coreclr/jit/morph.cpp Show resolved Hide resolved
src/coreclr/jit/morph.cpp Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Mar 29, 2022
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak
Copy link
Member

The tasks seems to be complete but not sure why it is not reflecting in GH. I will go ahead and merge this.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants