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.

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

Improve fgValueNumberBlockAssignment #64110

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Improve fgValueNumberBlockAssignment #64110

merged 1 commit into from
Feb 3, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jan 21, 2022 •
edited
Loading

fgValueNumberBlockAssignment had a very interesting oddity: it re-VNd the source tree of the assignment. This is unnecessary, and is in fact a pessimization, as it means we will fail to VN assignments from sources it does not understand. This change fixes that, bringing along some positive diffs from numbering stores from field indirections.

fgValueNumberBlockAssignment also needs to maintain the invariant that a location's VN will always match its type.
It was failing to do that in cases where the assignment's source was not local. This change fixes that.

Finally, this change unifies the code common to numbering CopyBlk and InitBlk cases. There is no need for them to be different.

Some positive diffs from the aforementioned more precise numbering.

Part of #58312.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 21, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed community-contribution Indicates that the PR has been added by a community member labels Jan 21, 2022
@ghost
Copy link

ghost commented Jan 21, 2022

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

Issue Details

fgValueNumberBlockAssignment had a very interesting oddity: it re-VNs the source tree of the assignment. This is unnecessary, and is in fact a pessimization, as it means we will fail to VN assignments from sources it does not understand. This change fixes that, bringing along some positive diffs from numbering stores from field indirections.

fgValueNumberBlockAssignment also needs to maintain the invariant that a location's VN will always match its type.
It was failing to do that in cases where the assignment's source was not local. This change fixes that.

Finally, this change unifies the code common to numbering CopyBlk and InitBlk cases. There is no need for them to be different.

We're expecting some positive diffs from the aforementioned more precise numbering.

Part of #58312.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review January 22, 2022 12:20
@SingleAccretion SingleAccretion changed the title Refactor fgValueNumberBlockAssignment Improve fgValueNumberBlockAssignment Jan 22, 2022
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Jan 25, 2022
@SingleAccretion
Refactor fgValueNumberBlockAssignment
4601ecc 
"fgValueNumberBlockAssignment" had a very interesting oddity:
it re-VNs the source tree of the assignment. This is unnecessary,
and is in fact a pessimization, as it means we will fail to VN
assignments from sources it does not understand. This change
fixes that, bringing along some positive diffs from numbering
stores from field indirections.

"fgValueNumberBlockAssignment" also needs to maintaint the
invariant that a location's VN will always match its type.
It was failing to do that in cases where the assignment's
source was not local. This change fixes that.

Finally, this change unifies the code common to numbering
"CopyBlk" and "InitBlk" cases. There is no need for them to
be different.
@jakobbotsch
Copy link
Member

/azp run Antigen, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch
jakobbotsch approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch!

@jakobbotsch jakobbotsch merged commit c85f02d into dotnet:main Feb 3, 2022
@SingleAccretion SingleAccretion deleted the Simplify-fgVNBlkAsg branch February 3, 2022 21:02
@SingleAccretion SingleAccretion mentioned this pull request Feb 4, 2022
Closed
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Mar 3, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@jakobbotsch jakobbotsch

@SingleAccretion SingleAccretion

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@SingleAccretion @jakobbotsch @JulieLeeMSFT