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

JIT: Remove BBJ_RETURN loop cloning quirk #96555

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

jakobbotsch
Copy link
Member

With the new loop representation there will never be any BBJ_RETURN blocks in loops we are cloning -- add an assert for this. Then remove the quirk that avoids cloning loops when the old loop cloning would create too many BBJ_RETURN blocks.

Large size wise regressions are expected due to new cloning.

With the new loop representation there will never be any `BBJ_RETURN`
blocks in loops we are cloning -- add an assert for this. Then remove
the quirk that avoids cloning loops when the old loop cloning would
create too many `BBJ_RETURN` blocks.

Large size wise regressions are expected due to new cloning.
@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 5, 2024
@ghost ghost assigned jakobbotsch Jan 5, 2024
@ghost
Copy link

ghost commented Jan 5, 2024

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

Issue Details

With the new loop representation there will never be any BBJ_RETURN blocks in loops we are cloning -- add an assert for this. Then remove the quirk that avoids cloning loops when the old loop cloning would create too many BBJ_RETURN blocks.

Large size wise regressions are expected due to new cloning.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @BruceForstall

Large size regressions. Looks like this check was preventing some loop cloning in ArraySortHelper.IntroSort from happening; a lot of the diffs seem to be repeated diffs in that function:
image

The check is based on fgReturnCount, which does not seem to be tracked correctly. In the base JITDUMP I see the following:

Considering loop L00 to clone for optimizations.
Loop cloning: rejecting loop L00. It has 0 returns; if added to previously existing -4 returns, it would exceed the limit of 4.

In the diff we no longer reject it.

Naturally these changes are going to allow more loop cloning, and without profitability heuristics that's going to result in size/TP regressions. I think it means we should prioritize adding profitability heuristics as part of .NET 9. Luckily #8558 is already in 9.0 and tracks that.

@jakobbotsch jakobbotsch marked this pull request as ready for review January 5, 2024 17:10
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.

Looks good!

@jakobbotsch jakobbotsch merged commit 24bc1b7 into dotnet:main Jan 6, 2024
126 of 129 checks passed
@jakobbotsch jakobbotsch deleted the loop-cloning-quirks-3 branch January 6, 2024 10:49
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants