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

Keep the volatility of CLS_VARs in rationalization #65919

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Feb 26, 2022

Some expected negative diffs on ARM.

One more step towards CLS_VAR deletion.

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

ghost commented Feb 26, 2022

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

Issue Details

We're expecting some negative diffs on ARM.

One more step towards CLS_VAR deletion.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review February 26, 2022 16:37
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

if (isVolatile)
{
assignment->gtFlags |= GTF_IND_VOLATILE;
}

// TODO: JIT dump
Copy link
Member

Choose a reason for hiding this comment

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

wonder what that is supposed to mean 😄

@@ -453,11 +453,18 @@ void Rationalizer::RewriteAssignment(LIR::Use& use)

case GT_CLS_VAR:
{
bool isVolatile = (location->gtFlags & GTF_CLS_VAR_VOLATILE) != 0;

location->gtFlags &= ~GTF_CLS_VAR_VOLATILE;
Copy link
Member

Choose a reason for hiding this comment

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

is it required to reset GTF_CLS_VAR_VOLATILE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly, no one will look at it once it becomes CLS_VAR_ADDR, but it's good practice to clear flags when bashing to an oper that doesn't support them.

@AndyAyersMS AndyAyersMS added the community-contribution Indicates that the PR has been added by a community member label Feb 26, 2022
@JulieLeeMSFT JulieLeeMSFT merged commit 5d2da71 into dotnet:main Mar 4, 2022
@JulieLeeMSFT JulieLeeMSFT added this to the 7.0.0 milestone Mar 4, 2022
@SingleAccretion SingleAccretion deleted the ClsVar-Volatile branch March 4, 2022 21:42
@ghost ghost locked as resolved and limited conversation to collaborators Apr 4, 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