Tune GlobalAP for better jit diffs #123856
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the Global Assertion Propagation (GlobalAP) phase of the JIT compiler by removing throughput-heavy (TP-heavy) logic that provided minimal value. The changes trade a small code size regression (+35 bytes on benchmarks.run, +2700 overall) for improved compilation throughput, with the saved budget enabling additional assertions to catch issues earlier.
Changes:
- Simplified assertion count calculation from IL-size-based to basic block count-based heuristic
- Removed value-number-to-assertions mapping infrastructure (optValueNumToAsserts, optAddVnAssertionMapping, optGetVnMappedAssertions)
- Removed implied assertion generation logic (optImpliedAssertions, optImpliedByTypeOfAssertions, optImpliedByConstAssertion)
- Removed redundant NaN filtering in optAssertionVnInvolvesNan (NaN check already exists at assertion creation)
- Added defensive NaN assertion in optDebugCheckAssertion for O2K_CONST_DOUBLE
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/rangecheck.cpp | Removed obsolete comment referencing the removed optImpliedAssertions function |
| src/coreclr/jit/compiler.h | Removed declarations for ValueNumToAssertsMap infrastructure and implied assertion functions, with formatting cleanup |
| src/coreclr/jit/assertionprop.cpp | Simplified optAssertionInit with new assertion count heuristic; removed VN-to-assertions mapping code, NaN pre-filtering, and all implied assertion generation logic; added NaN assertion check in optDebugCheckAssertion; removed calls to removed functions throughout |
| // | ||
| static const AssertionIndex countFunc[] = {64, 128, 256, 128, 64}; | ||
| static const unsigned upperBound = ArrLen(countFunc) - 1; | ||
| const unsigned codeSize = info.compILCodeSize / 512; |
There was a problem hiding this comment.
compILCodeSize is not a reliable metric - we don't even sum it with inlinees. fgBBcount is a bit more reliable. Blocks might be huge, but I didn't want to complicate the complexity check. Long term I think we should unconditionally enable it for all sizes to be 256 once we improve a couple of slow things.
|
|
||
| // Even though the propagation step takes care of NaN, just a check | ||
| // to make sure there is no slot involving a NaN. | ||
| if (optAssertionVnInvolvesNan(newAssertion)) |
There was a problem hiding this comment.
We never produce constant assertions with NaN values anyway, moved to optDebugCheckAssertion
|
@AndyAyersMS @dotnet/jit-contrib PTAL (see main descr) |
|
I'm wondering with this change can we finally enable #101763 again? |
Not sure, I changed the Global Assertion Prop while that PR used Local Assertion Prop |
2 participants
Removed a bunch of TP-heavy logic with minimal value (e.g.
benchmarks.runregressed by +35 bytes, overall regression is +2700). Enabled more assertions to eat the saved TP budget (saved around -0.3% of TP).diffs - I think +0.1..+0.2% TP regression worth the diffs
Note: with
optMaxAssertionCountbeing unconditionally 1024 we get -600kb diffs, so if we make GAP more TP efficient we can get another -300kbPS: I might add my own
HashSet<VN>to perform O(1) lookups for "do we have an assertion for this VN" separately. WhileoptGetVnMappedAssertionssort of did that - it wasn't used and it tried to return ASSERT_TP for VN which made it slower and still can't be used everywhere -- it was not context-dependent. For that, I need #123845 to land first (so e.g. I can make sure nobody mutates assertions in an unexpected way due toconst AssertionDsc&everywhere).