JIT: Add post-phase validation of DFS tree #95682
Conversation
Add a post-phase check that validates that the DFS tree matches the current flow graph when it is non-null. Add a `fgInvalidateDfsTree()` that invalidates the DFS tree and derived annotations on it (dominators and loops). We invalidate after RBO, which is the last point we expect to make use of the flow graph information. Compute new DFS and loops as part of loop unrolling to make the DFS tree correct after this point too. Some TP regressions are expected due to this. To sum up we now keep this information valid from loop finding until RBO. Fix dotnet#95652
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAdd a post-phase check that validates that the DFS tree matches the current flow graph when it is non-null. Add a Compute new DFS and loops as part of loop unrolling to make the DFS tree correct after this point too. Some TP regressions are expected due to this. To sum up we now keep this information valid from loop finding until RBO. Fix #95652
|
| // Assertion prop can do arbitrary statement remorphing, which | ||
| // can clone code and disrupt our simpleminded SSA accounting. | ||
| // | ||
| // So, disable the ssa checks. | ||
| // | ||
| if (fgSsaValid) | ||
| { | ||
| JITDUMP("Marking SSA as invalid before assertion prop\n"); | ||
| fgSsaValid = false; | ||
| } |
There was a problem hiding this comment.
@AndyAyersMS The SSA checker is dependent on SSA's dominator tree, which is stale after RBO, so I'm also setting fgSsaValid = false in fgInvalidateDfsTree. It means we no longer check after CSEs, but I suppose that's not a big issue since RBO is really the last user.
There was a problem hiding this comment.
(Seems like the SSA checker could be switched to iterate the post order of the DFS as well)
There was a problem hiding this comment.
Ok by me -- at one point I wanted to keep the SSA checked properties viable longer, but I don't recall why.
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| { | ||
| visitor.PreOrderVisit(block); | ||
| scv.WalkTree(stmt->GetRootNodePointer(), nullptr); | ||
| } | ||
| } |
There was a problem hiding this comment.
I've removed this since fgDomBBcount is not SSA's dom count, and since the new DFS validation would trigger if a (reachable) block was added anyway. Also switched the above to iterate the blocks via the computed post order from the DFS tree.
| // The DFS we run is not precise around call-finally, so | ||
| // `fgRemoveUnreachableBlocks` can expose newly unreachable blocks | ||
| // that we did not uncover during the DFS. If we did remove any | ||
| // call-finally blocks then iterate to closure. This is a very rare | ||
| // case. | ||
| while (true) | ||
| { | ||
| bool anyCallFinallyPairs = false; | ||
| fgRemoveUnreachableBlocks([=, &anyCallFinallyPairs](BasicBlock* block) { | ||
| if (!m_dfsTree->Contains(block)) | ||
| { | ||
| anyCallFinallyPairs |= block->isBBCallAlwaysPair(); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
|
|
||
| if (!anyCallFinallyPairs) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| m_dfsTree = fgComputeDfs(); | ||
| } |
There was a problem hiding this comment.
Sadly even with the more precise EH treatment we do not get around this issue, so I've had to add this iteration to closure to make sure the validation passes in cases where removing a callfinally/always pair results in new unreachable blocks (without the validation it would just result in a bit conservative behavior -- considering some blocks reachable that aren't). I only saw one assertion in all of our collections, so at least TP wise it does not seem like an issue.
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
The Fuzzlyn failure repros on main too, will open an issue for that. cc @dotnet/jit-contrib PTAL @AndyAyersMS @BruceForstall |
Add a post-phase check that validates that the DFS tree matches the current flow graph when it is non-null.
Add a
fgInvalidateDfsTree()that invalidates the DFS tree and derived annotations on it (dominators and loops). We invalidate after RBO, which is the last point we expect to make use of the flow graph information.Fix #95652