JIT: fix profile propagation in LowerSwitch - #130907
#130907JIT: fix profile propagation in LowerSwitch#130907AndyAyersMS merged 1 commit intodotnet:maindotnet/runtime:mainfrom AndyAyersMS:fix-130785-cond-collapse-likelihoodAndyAyersMS/runtime:fix-130785-cond-collapse-likelihoodCopy head branch name to clipboard
Conversation
|
@EgorBo PTAL |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR adjusts CoreCLR JIT profile-consistency handling in two places: (1) it adds a “dump-only” mode to the debug profile-weight checker so it can log details before asserting, and (2) it ensures edge-likelihood invariants are maintained when conditional branches are collapsed to unconditional branches.
Changes:
- Extend
fgDebugCheckProfileWeightswith adumpmode and re-run the check withverbose=trueto emit diagnostics prior to asserting on inconsistent profile data. - Normalize the successor edge likelihood when converting a degenerate
BBJ_CONDtoBBJ_ALWAYS. - Update the
Compilerdeclaration to match the newfgDebugCheckProfileWeightssignature.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/fgprofile.cpp | Adds dump mode and a verbose re-run to print profile inconsistency details before asserting. |
| src/coreclr/jit/fgopt.cpp | Adjusts edge likelihood handling when removing a conditional jump that always targets the same block. |
| src/coreclr/jit/compiler.h | Updates the fgDebugCheckProfileWeights declaration to include the new optional dump parameter. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/fgprofile.cpp:4867
- In dump mode, the
unflaggedBlockspath is completely suppressed (no JITDUMP, no assert). This defeats the purpose of re-running withdump=trueto log details before asserting, since missingBBF_PROF_WEIGHTflags will still assert without any diagnostic output whenverbosewas originally false. Consider always emitting the JITDUMP, and only gating the assert on!dump.
if ((unflaggedBlocks > 0) && !dump)
{
JITDUMP("%d blocks are missing BBF_PROF_WEIGHT flag.\n", unflaggedBlocks);
assert(!"Missing BBF_PROF_WEIGHT flag");
}
When peeling a fully-biased default case, recompute each remaining switch target's weight from its incoming edges instead of adjusting it incrementally, so return targets (no successors) don't keep a stale weight and trip the profile check. Also add an assert-time diagnostic that dumps the offending block. Fixes dotnet#130785 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f8414be-99cb-492d-a7c0-091c8e7de191
c8b1987 to
b78cb45
Compare
|
@EgorBo can you re-review. Got a repro finally. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "b78cb450324ea0383476421f9d2765e81c671a4b",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "e7c3d6567c4d78e0c10627fa89939767464478c2",
"last_reviewed_commit": "b78cb450324ea0383476421f9d2765e81c671a4b",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "e7c3d6567c4d78e0c10627fa89939767464478c2",
"last_recorded_worker_run_id": "29688126480",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "b78cb450324ea0383476421f9d2765e81c671a4b",
"review_id": 4730808760
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Fixes #130785, an Inconsistent profile data assertion in Lowering. When LowerSwitch peels the default case and then distributes case likelihoods equally (the zero-remaining-likelihood path), it was updating each switch target's block weight incrementally via increaseBBProfileWeight(newEdgeWeight - oldEdgeWeight). Because the earlier default-peel had already scaled afterDefaultCondBlock's weight but left the target weights stale, the incremental delta accumulated on top of a stale base, producing inconsistent profile weights and tripping the debug checker.
Approach: In the equal-distribution loop, the target's weight is now recomputed from scratch from its incoming edges: targetBlock->setBBProfileWeight(targetBlock->computeIncomingWeight()), replacing the fragile incremental adjustment. This mirrors the established pattern already used across the JIT after edge-likelihood changes (optimizebools.cpp, optimizer.cpp, helperexpansion.cpp). The now-unused oldEdgeWeight/newEdgeWeight locals are removed. Separately, fgDebugCheckProfileWeights gains a dump parameter (debug-only): before asserting on inconsistency, it re-runs itself with dumping forced on so the offending blocks are logged in the JIT dump prior to the assert; the missing-flag assert and the recursion are correctly guarded by !dump to avoid double-assert/infinite recursion.
Summary: This is a small, well-targeted correctness fix in profile-weight propagation plus a helpful debug diagnostic. The recompute-from-incoming-edges approach is consistent with existing JIT conventions and is robust against stale base weights. The dump re-run recursion terminates (guarded by !dump) and restores verbose. Note the sibling scaleFactor branch (the non-zero remaining-likelihood path) only adjusts edge likelihoods and does not touch block weights, so it is unaffected by this class of bug. I found no correctness, performance, or convention issues in the changed lines. Behavior in release builds is limited to the one-line weight recomputation; the rest is DEBUG-only. LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 60.6 AIC · ⌖ 10.5 AIC · ⊞ 10K
Recompute switch target block weights rather than trying to fix them incrementally.
Fixes #130785