Remove PHASE_MERGE_THROWS#129084
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR removes the standalone throw-block tail-merge phase (PHASE_MERGE_THROWS / Compiler::fgTailMergeThrows) and folds its intent into the existing PHASE_HEAD_TAIL_MERGE implementation, expanding terminal (return/throw) tail-merging behavior.
Changes:
- Adjusts
fgHeadTailMergeto apply the predecessor-count cap only for common-successor tail merges, and to retry terminal (return/throw) merges until no more groups are found. - Deletes
fgTailMergeThrowsand all related plumbing (phase entry, mem kind, compiler.h declaration, phase invocation). - Removes the
PHASE_MERGE_THROWSphase name andTailMergeThrowsallocation kind.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/fgopt.cpp | Updates head/tail merge to retry terminal merges and exempts terminal merges from the pred-count cap. |
| src/coreclr/jit/fgehopt.cpp | Removes the old fgTailMergeThrows implementation entirely. |
| src/coreclr/jit/compphases.h | Removes PHASE_MERGE_THROWS from the phase list. |
| src/coreclr/jit/compmemkind.h | Removes TailMergeThrows memory kind. |
| src/coreclr/jit/compiler.h | Removes the fgTailMergeThrows declaration. |
| src/coreclr/jit/compiler.cpp | Removes the DoPhase(... PHASE_MERGE_THROWS ...) invocation. |
# Conflicts: # src/coreclr/jit/fgopt.cpp
Bounds the terminal-block merge work instead of leaving it unlimited. 2x (100) is the minimal multiple that avoids code-size regressions vs the fgTailMergeThrows baseline (SPMI asmdiffs over libraries.pmi/benchmarks/aspnet); 1x regresses +5,982 bytes on libraries.pmi. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
PTAL cleanup @AndyAyersMS @dotnet/jit-contrib |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM
Interesting that there are diffs. I couldn't see what they were from. Maybe we need a tails merged metric.
Just from bumping the threshold maybe? I can add the metric if you think it's worth it (as part of some other change) |
PHASE_HEAD_TAIL_MERGE fully covers what PHASE_MERGE_THROWS did. Had to bump the threshold to cover some regressions.
-13kb diffs on win-x64