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

[release/8.0] Check if loop body occured before loopTop and if so unmark alignment #91918

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 12, 2023

Backport of #91854 to release/8.0

/cc @kunalspathak

Customer Impact

In some cases, we might falsely align loops that might adversely affect the performance. This PR fixes that problem.

Testing

Our CI includes the tests are they no longer fail with the fix. Also, manually verified that false loops are not marked as needs alignment.

Risk

Low because this happens in code having lot of loops and need lot of code motion to get this scenario triggered.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@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 Sep 12, 2023
@ghost
Copy link

ghost commented Sep 12, 2023

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

Issue Details

Backport of #91854 to release/8.0

/cc @kunalspathak

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member

@AndyAyersMS

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. please get a code review. once ready this can be merged.

@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Sep 12, 2023
@jeffschwMSFT jeffschwMSFT added this to the 8.0.0 milestone Sep 12, 2023
@BruceForstall
Copy link
Member

This PR has an issue that needs to be addressed first:

#91972

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 12, 2023
@carlossanlop carlossanlop added the blocked Issue/PR is blocked on something - see comments label Sep 14, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 15, 2023
@kunalspathak
Copy link
Member

@BruceForstall - Updated the PR with changes from #92132. @carlossanlop - we should wait to merge this PR only after #92132 has a green CI and is merged (in sometime).

@kunalspathak
Copy link
Member

@carlossanlop - #92132 is merged. We can merge this PR as well.

@carlossanlop
Copy link
Member

carlossanlop commented Sep 18, 2023

@kunalspathak @BruceForstall @jeffschwMSFT I'm confused: PR #92132 was merged to main, but this PR is for release/8.0. Do we need to backport #92132 before merging this?

@BruceForstall
Copy link
Member

@carlossanlop This backport PR was already updated with the changes in #92132. So, this backport PR includes the changes from both PR #91854 as well as the follow-up PR #92132.

@carlossanlop
Copy link
Member

Got it, I misunderstood. Merging now.

@carlossanlop carlossanlop merged commit bf1e333 into release/8.0 Sep 18, 2023
@carlossanlop carlossanlop deleted the backport/pr-91854-to-release/8.0 branch September 18, 2023 18:31
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 18, 2023
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 blocked Issue/PR is blocked on something - see comments Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants