Revert "Revert 'Increase number of assertions (GlobalAP) + VN cache (#124132)'"#125215
Merged
Revert "Revert 'Increase number of assertions (GlobalAP) + VN cache (#124132)'"#125215
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
…124132)'" Re-applies the changes from PR #124132 that were reverted in PR #124928 due to a bug causing excessive memory allocation during jitting. The bug has since been fixed. Changes: - Formula-based optMaxAssertionCount heuristic (basicBlocks + tracked locals) - VN cache (optAssertionHasAssertionsForVN) for fast O(1) lookup - optAssertionVNsMap field added to Compiler - optAddAssertion uses VN cache for duplicate detection/registration - Early return in optAssertionProp_LclVar when no assertions for VN - VN cache check in optAssertionVNIsNonNull - rangecheck.cpp uses optAssertionHasAssertionsForVN instead of NoVN check - Includes addOpVN pattern fix from PR #124306 Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Revert changes from PR 124928 after fixing uncovered bug
Revert "Revert 'Increase number of assertions (GlobalAP) + VN cache (#124132)'"
Mar 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR re-applies the changes from #124132 (which was reverted in #124928 due to excessive JIT memory allocation). It improves the JIT's assertion propagation by adding a VN (Value Number) existence cache for O(1) lookups and refining the heuristic for sizing the global assertion table.
Changes:
- Replaces the IL-code-size step-function table for
optMaxAssertionCountwith an empirically-validated formula based on tracked local count and basic block count, yielding assertion table sizes between 64 and 256. - Adds
optAssertionVNsMap(a lazily-initializedVNSet*cache) andoptAssertionHasAssertionsForVN()to enable O(1) checks for whether any assertions exist for a given VN, avoiding expensive O(n) table scans inoptAddAssertion,optAssertionProp_LclVar,optAssertionVNIsNonNull, andMergeEdgeAssertions. - Includes the fix from #124306: registers the decomposed
VNF_ADDoperand VN in the cache soMergeEdgeAssertionscan find matching assertions for theVN + constpattern.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/jit/compiler.h |
Adds VNSet* optAssertionVNsMap field and declares optAssertionHasAssertionsForVN() method. |
src/coreclr/jit/assertionprop.cpp |
Replaces assertion table sizing heuristic; adds VN cache gating for duplicate detection in optAddAssertion, early-out in optAssertionProp_LclVar and optAssertionVNIsNonNull; implements optAssertionHasAssertionsForVN(). |
src/coreclr/jit/rangecheck.cpp |
Replaces the NoVN guard in MergeEdgeAssertions with the VN cache check, which also handles NoVN internally. |
Member
|
PTAL @AndyAyersMS @dotnet/jit-contrib This PR reverts the revert as the issue was fixed. Diffs |
AndyAyersMS
approved these changes
Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Diffs
Re-applies #124132 which was reverted in #124928 due to excessive JIT memory allocation. That root cause has since been fixed.
Description
assertionprop.cpp/compiler.h— VN existence cacheoptAssertionVNsMap(VNSet*): lazily-initialized map tracking which VNs have at least one assertionoptAssertionHasAssertionsForVN(vn, addIfNotFound): O(1) lookup; whenaddIfNotFound=trueregisters the VN on first insertionoptAddAssertion: uses the cache to gate the full O(n) table scan for duplicates; also registersop2bound VN and theVN + constoperand VN (fix from #124306) soMergeEdgeAssertionscan find themoptAssertionProp_LclVar: early-outs via cache before iterating assertions; pre-computestreeVNonce instead of re-deriving it in the loopoptAssertionVNIsNonNull: skips iteration when cache says no assertions exist for the VNassertionprop.cpp—optMaxAssertionCountheuristicrangecheck.cppMergeEdgeAssertions: replacesnormalLclVN == NoVNguard with!optAssertionHasAssertionsForVN(normalLclVN)— handles theNoVNcase internally and also skips the loop when no relevant assertions were ever registered.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.