Skip to content

Code cleanup: remove TODO-ASG with zero diff quirk in lclmorph #106889

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
Closed
wants to merge 1 commit into from
Closed

Code cleanup: remove TODO-ASG with zero diff quirk in lclmorph #106889

wants to merge 1 commit into from

Conversation

JulieLeeMSFT
Copy link
Member

Remove TODO comment because ASG work is done.

//node->gtFlags &= (GTF_NODE_MASK | GTF_DONT_CSE); // TODO-ASG-Cleanup: delete this zero-diff quirk.

SPMI replay is clean, and asmdiffs has more improvements and regressions look fine.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 23, 2024
@JulieLeeMSFT JulieLeeMSFT added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 23, 2024
@JulieLeeMSFT JulieLeeMSFT added this to the 10.0.0 milestone Aug 23, 2024
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@JulieLeeMSFT
Copy link
Member Author

GTF_NODE_MASK and GTF_COLON_COND have the same value, but they represent different things. We should not remove one of them.
cc @jakobbotsch, @amanasifkhalid.

amanasifkhalid
amanasifkhalid approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

Most of the size regressions are concentrated in coreclr_tests and libraries_tests. LGTM, thanks!

@build-analysis build-analysis bot mentioned this pull request Aug 23, 2024
Closed
kasperk81
kasperk81 reviewed Aug 23, 2024
View reviewed changes
src/coreclr/jit/lclmorph.cpp
else
{
node->ChangeOper(GT_LCL_VAR);
node->gtFlags &= (GTF_NODE_MASK | GTF_DONT_CSE); // TODO-ASG-Cleanup: delete this zero-diff quirk.
Copy link
Contributor

Choose a reason for hiding this comment

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

@SingleAccretion's pr is replacing it with empty node https://github.com/dotnet/runtime/pull/86726/files#diff-bd3095cf3bd9e41e7df84eb759f6983966aed4f2baab6179cd7759dfbcca3c1a

Copy link
Member Author

Choose a reason for hiding this comment

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

@kasperk81, thanks for pointing me to @SingleAccretion's PR.
I see that he is still working on the PR. So, I will close mine.

@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Aug 23, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kasperk81 kasperk81 kasperk81 left review comments

@amanasifkhalid amanasifkhalid amanasifkhalid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JulieLeeMSFT JulieLeeMSFT

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@JulieLeeMSFT @amanasifkhalid @kasperk81