JIT: Port loop compaction to new loop representation #96995
Conversation
Some diffs expected from (at least) two sources: - Less/different compaction in cases where new loop recognition recognizes fewer loops than old recognition - Less compaction in cases where compacting the first loop led to old loop recognition recognizing another loop after the first one. This leads to no `BBF_OLD_LOOP_HEADER_QUIRK` set on the second loop's header found by new loop finding, meaning we do not compact it. It will be compacted as part of a future PR that enables compaction for all loops. - Slight differences in compaction behavior for blocks that aren't part of the loop and that couldn't be moved, which would previously abort compaction and recognition of the loop. Now we just leave those blocks in place.
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsSome diffs expected from (at least) three sources:
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @BruceForstall Some minor codegen diffs, see above for why. Ideally we remove this entire compaction step once we get a new block layout algorithm in the backend, so mostly I view this code for now as a way to reduce churn. Unfortunately we will probably see a bunch of diffs when we enable the compaction for new loops as well, but I don't see a simple way to avoid that if we want to be able to remove old loop finding.
|
src/coreclr/jit/optimizer.cpp
Outdated
| if (optCompactLoops()) | ||
| { | ||
| fgInvalidateDfsTree(); | ||
| m_dfsTree = fgComputeDfs(); | ||
| m_loops = FlowGraphNaturalLoops::Find(m_dfsTree); | ||
| modified = true; | ||
| } |
There was a problem hiding this comment.
The only reason this is needed is because optCompactLoops needs to insert blocks to fix up fallthrough. Otherwise the DFS tree would not be invalidated by it. It would be possible to avoid this by waiting with fixing up the fall through until after we have also canonicalized the loops, e.g. by having some fgFixFallthrough like we discussed the other day.
| BasicBlock* previous = cur->Prev(); | ||
| BasicBlock* nextLoopBlock = lastNonLoopBlock->Next(); | ||
| assert(previous != nullptr); | ||
| if (!BasicBlock::sameEHRegion(previous, nextLoopBlock) || !BasicBlock::sameEHRegion(previous, insertionPoint)) |
There was a problem hiding this comment.
fyi: this code is basically pre-existing, but there's an issue for fixing/improving it: #85550
Some diffs expected from (at least) three sources: - Less/different compaction in cases where new loop recognition recognizes fewer loops than old recognition - Less compaction in cases where compacting the first loop led to old loop recognition recognizing another loop after the first one. This leads to no `BBF_OLD_LOOP_HEADER_QUIRK` set on the second loop's header found by new loop finding, meaning we do not compact it. It will be compacted as part of a future PR that enables compaction for all loops. - Slight differences in compaction behavior for blocks that aren't part of the loop and that couldn't be moved, which would previously abort compaction and recognition of the loop. Now we just leave those blocks in place.
Some diffs expected from (at least) three sources:
BBF_OLD_LOOP_HEADER_QUIRKset on the second loop's header found by new loop finding, meaning we do not compact it. It will be compacted as part of a future PR that enables compaction for all loops.