JIT: Move loop inversion to after loop recognition #115850
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
area-CodeGen-coreclr
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR moves the loop inversion phase to after loop recognition, adds immediate block compaction/removal for newly altered test blocks, and triggers a DFS rebuild with fresh loop analysis when any loops were inverted.
- Add single-predecessor block compaction/removal in
optInvertWhileLoop - Recompute the DFS tree and re-run loop finding after any loop inversions
- Relocate the
PHASE_INVERT_LOOPScall in the compilation pipeline
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| optimizer.cpp | Inserted block compaction/removal and DFS invalidation |
| compiler.cpp | Moved the loop inversion phase to a later point in compCompile |
Comments suppressed due to low confidence (1)
src/coreclr/jit/compiler.cpp:4668
- Add targeted tests that verify the new phase ordering and ensure that both block compaction and removal occur as expected after loop inversion.
DoPhase(this, PHASE_INVERT_LOOPS, &Compiler::optInvertLoops);
Open
3 tasks
Contributor
Author
|
The diffs will be hard to parse for this, so I'm looking more at metrics. Here are some metric diffs for Base:
Diff:
We can see from the metrics that we're inverting fewer loops overall, but there are plenty of cases where we invert new loops, thus unblocking other loop opts -- in particular, we're doing a lot more cloning. Fewer loops found overall is due to loop inversion no longer introducing new cycles before loop recognition runs. PerfScore diffs are overwhelmingly negative in non-PGO collections. This might be heuristic-derived profile weights for cloned loops inflating PerfScores, and/or something else... |
Member
|
Assuming the diffs are largely cloning related, it appears that extra cloning is pretty costly. It is hard to know how much of it is really beneficial. I wish we had better heuristics. |
Contributor
Author
Right, because of this, I've decided to flip my ordering and enable graph-based loop inversion with the existing phase ordering. Locally, the diffs are slightly easier to triage. Once that's in, hopefully it'll be easier to triage the diffs on this PR and see if there's anything actionable. |
AndyAyersMS
approved these changes
Jun 4, 2025
Contributor
Author
Member
Contributor
Author
|
I'm removing |
Contributor
Author
|
Diffs show yet another round of large size increases, though most of this seems to be driven by Diffs in our non-test collections, particularly the ones with Dynamic PGO enabled, are much less dramatic. Also, the TP improvement pays for #116017, which is nice. @AndyAyersMS are you ok with this going into Preview 6? |
Contributor
Author
|
/ba-g unrelated wasm build failure, and a known issue |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisite to #113709. I expect diffs to go both ways: In some cases, loop canonicalization unlocks pattern-based loop inversion, whereas in other cases, we now recognize fewer loops due to loop inversion no longer introducing new cycles pre-canonicalization.