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

Copy propagation tweaking #64378

Merged
merged 4 commits into from
Feb 10, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jan 27, 2022

Some small CQ improvements in preparation for a zero-diff TP-oriented refactoring.

Please refer to the individual commit messages for details.

Diffs.

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

ghost commented Jan 27, 2022

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

Issue Details

Some small CQ improvements in preparation for a zero-diff TP-oriented refactoring.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review January 27, 2022 15:51
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

LGTM

The type check is too conservative: it prevents partial
definitions from being used in propagation:
```
LCL_FLD V00/1 [X] = { ... }; // Pushed on the stack as a def.

USE LCL_VAR V01 // Has the same VN as V00/1, but the type
                // check prevented it from being replaced.
```
This new version is conservative too, but will do for now
as we don't propagate on (most) partial uses.

Another reason for this change is that in my upcoming refactoring
of copy propagation (that will bring another 0.5% in TP gains), we
will no longer have the "defNode" available.
Ordinarily, shadowed parameters would not be used for
propagation anyway, because of the liveness check, but
"this" bypasses that checks, and so was used, which is
presumably not what we want.

Regardless of that, it is also not profitable to propagate
"this" in such a situation as it extends its live range
and makes the RA unhappy.
@@ -10858,6 +10858,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
public:
struct ShadowParamVarInfo
{
static const unsigned NO_SHADOW_COPY = UINT_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: why not just use BAD_VAR_NUM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose no reason. Deleted it.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks again for your ongoing contributions.

@AndyAyersMS AndyAyersMS merged commit 161f13d into dotnet:main Feb 10, 2022
@SingleAccretion SingleAccretion deleted the Copy-Propagation-Checks branch February 10, 2022 21:08
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 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.

4 participants