JIT: fix issue in assertion prop phi inference #116326
Conversation
Unreachable preds may give conflicting assertions; check both for an assertion that agrees and the absence of one that disagrees. Also add the ability to dump the flow graph via hash. Fixes dotnet#116212.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@EgorBo PTAL Open to other ideas on how to fix this, though this one has fairly minimal diffs. |
There was a problem hiding this comment.
Pull Request Overview
This PR resolves an assertion propagation bug by checking for both null and non-null assertions and adds new JIT configuration flags to dump the flow graph by method hash or tier-0 compilations.
- Introduce
JitDumpFgHashandJitDumpFgTier0flags and wire them into the flow-graph dump logic. - Add
Compiler::CanPropNull()and update assertion propagation to handle conflicting null assertions correctly. - Add a JIT regression test (Runtime_116212) to reproduce and guard against the original issue.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_116212/Runtime_116212.csproj | Add project file for the new regression test |
| src/tests/JIT/Regression/JitBlue/Runtime_116212/Runtime_116212.cs | Add test case invoking the problematic pattern |
| src/coreclr/jit/jitconfigvalues.h | Add JitDumpFgHash and JitDumpFgTier0 configuration flags |
| src/coreclr/jit/fgdiagnostic.cpp | Include hash and tier-0 checks in flow-graph dump decision |
| src/coreclr/jit/compiler.h | Add CanPropNull() helper alongside CanPropNonNull() |
| src/coreclr/jit/assertionprop.cpp | Refactor null/non-null assertion logic in optAssertionVNIsNonNull |
Comments suppressed due to low confidence (3)
src/coreclr/jit/jitconfigvalues.h:266
- [nitpick] The comment for
JitDumpFgHashis generic; clarify that this flag filters dumps by matching the method hash (e.g., "Only dump flowgraphs when method hash equals the configured value").
CONFIG_INTEGER(JitDumpFgHash, "JitDumpFgHash", 0) // Dumps Xml/Dot Flowgraph for specified method
src/coreclr/jit/compiler.h:7876
- [nitpick] Add a brief comment above
CanPropNull()explaining its purpose and how it pairs withCanPropNonNull(), to improve maintainability and API clarity.
bool CanPropNull()
src/coreclr/jit/fgdiagnostic.cpp:414
- The current tier-0 check only gates dumping for tier-0 compilations, but does not prevent dumping on non-tier0 when
JitDumpFgTier0is zero. Consider inverting or expanding this logic so that settingJitDumpFgTier0==0disables dumps for non-tier0 as well.
if (opts.IsTier0())
|
@EgorBo @jakobbotsch I changed this up a bit, take another look? |
| // | ||
| for (BasicBlock* const pred : ssaDef->GetBlock()->PredBlocks()) | ||
| { | ||
| if (!BitVecOps::IsMember(&traits, visitedBlocks, pred->bbNum)) |
There was a problem hiding this comment.
Doesn't it mean the SSA is basically stale?
There was a problem hiding this comment.
Yes, RBO messes up SSA, though in the past we've argued that it generally doesn't lead to making bad assumptions. This is one case where it does.
|
/ba-g installer build and test failed with no log. |
Unreachable preds may give conflicting assertions; check both for an assertion that agrees and the absence of one that disagrees.
Also add the ability to dump the flow graph via hash.
Fixes #116212.