JIT: Generalize check for cycles when finding loops #96153
Conversation
optFindNewLoops tries to leave a breadcrumb for loop alignment about whether it needs to bother with identifying loops. It does so by checking for whether any natural loops were found, but also whether any irreducible loops were found (i.e. any cycle). The idea is that optimizations that run between finding loops and loop alignment can easily remove edges that make irreducible loops into natural loops. However, the check for "any cycle" was not precise enough; it made the assumption that if we find no improper loop headers, this means there is no cycle. But that is not true, specifically because we only consider blocks to be loop headers when their backedges are non-exceptional. Odd loop structures can exist that iterate through required exception throws, like in the case of dotnet#96145. In that case we have a cycle through the handler, which ends up being a natural loop after some optimizations run. To fix the situation we generalize `fgComputeDfs` to identify when the flow graph has a cycle, which is the fully general property we want for the breadcrumb. As a bonus we can skip identifying loops if we know the flow graph has no cycles. Fix dotnet#96145
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Details
However, the check for "any cycle" was not precise enough; it made the assumption that if we find no improper loop headers, this means there is no cycle. But that is not true, specifically because we only consider blocks to be loop headers when their backedges are non-exceptional. Odd loop structures can exist that iterate through required exception throws, like in the case of #96145. In that case we have a cycle through the handler, which ends up being a natural loop after some optimizations run. (Flowgraph -- the cycle is To fix the situation we generalize Fix #96145
|
| BasicBlock* const block = dfsTree->GetPostOrder(i - 1); | ||
| JITDUMP("%02u -> " FMT_BB "[%u, %u]\n", rpoNum + 1, block->bbNum, block->bbPreorderNum + 1, | ||
| block->bbNewPostorderNum + 1); | ||
| JITDUMP("%02u -> " FMT_BB "[%u, %u]\n", rpoNum, block->bbNum, block->bbPreorderNum, block->bbNewPostorderNum); |
There was a problem hiding this comment.
I made this output 1-based to exactly match the original loop finding way back when I originally factored it out and was investigating a cause of diffs, but forgot to revert it back to be 0-based.
|
Originally I looked at simply generalizing loop finding to actually find the loop in the example above, which amounts to switching to use runtime/src/coreclr/jit/flowgraph.cpp Line 4408 in bdd9a47 This also fixes the problem, but the loop structure is so odd that I don't think we want to consider it a loop. One odd thing, for instance, is that it is impossible to canonicalize the loop to have a preheader. |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS @BruceForstall No diffs, only some minor TP ones. |
3 participants
optFindNewLoopstries to leave a breadcrumb for loop alignment about whether it needs to bother with identifying loops. It does so by checking for whether any natural loops were found, but also whether any irreducible loops were found (i.e. any cycle). The idea is that optimizations that run between finding loops and loop alignment can easily remove edges that make irreducible loops into natural loops.However, the check for "any cycle" was not precise enough; it made the assumption that
NumLoops() > 0 || NumImproperLoopHeaders > 0was equivalent to there being a cycle in the flow graph. But that is not true, specifically because we only consider blocks to be loop headers when their backedges are non-exceptional. Odd loop structures can exist that iterate through required exception throws, like in the case of #96145. In that case we have a cycle through the handler, which ends up being a natural loop after some optimizations run. (Flowgraph -- the cycle isBB08 -> BB09 -> BB12 -> BB05 -> BB08. The entry edge would beBB02 -> BB08.). But since the backedge is exceptional, we do not consider it as a loop during loop finding, and we do not consider it as an improper loop header either.To fix the situation we generalize
fgComputeDfsto identify when the flow graph has a cycle, which is the fully general property we want for the breadcrumb. As a bonus we can skip identifying loops if we know the flow graph has no cycles.Fix #96145