-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Increase number of assertions (GlobalAP) + VN cache #124132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR increases Global Assertion Propagation capacity and adds a small VN-based cache to avoid unnecessary full-table assertion scans, benefiting range check/range analysis and assertion propagation throughput.
Changes:
- Adjusts
optMaxAssertionCountheuristics to allow tracking more assertions (especially for GlobalAP). - Introduces
optAssertionVNsMap+optAssertionHasAssertionsForVN()to quickly determine if any assertions exist for a given VN (with lazy initialization). - Uses the VN cache to early-out in range-check assertion merges and GlobalAP local-var assertion propagation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/coreclr/jit/rangecheck.cpp |
Uses the new VN cache to skip assertion scanning when no assertions exist for the VN. |
src/coreclr/jit/compiler.h |
Adds optAssertionVNsMap and declares optAssertionHasAssertionsForVN(). |
src/coreclr/jit/assertionprop.cpp |
Updates assertion table sizing, populates/queries the VN cache, and applies new early-out paths in GlobalAP. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
|
@AndyAyersMS @dotnet/jit-contrib PTAL. This is still a hacky way (I mean the heuristic) to deal with the TP regression, but it's better than nothing. Very few regressions, lots of improvements, small TP regression overall. |
Diffs
This PR does two things:
I initially tried to use this cache everywhere, but ended up keeping it only in a few places where it had a noticeable TP benefit. This change alone has zero size diffs and reduces the TP regression from change (2) by 2×.
With help from SPMI, I analyzed 1M methods and built a large table:
| BB count | TrackedLocals count | assertions created |. I then fed it to an AI agent, which iterated for a while, generated some Python scripts, and proposed a heuristic (after several rounds of SPMI TP/size feedback). This is the best trade-off between improvements and TP regression that I was able to find. We could potentially incorporate more parameters (e.g., number of conditional blocks against null/constant, number of indirects), but I think the current version is good enough for now.* I had 3 variants and ended up picking the 3rd one due to better TP results (obviously, less of =256 entities)
NOTE: If I unconditionally set
optMaxAssertionCountto 256 the size diffs become ~8% better and TP regresses by 3xThe worst-case +0.3% TP regression should be more than covered by my earlier refactoring PRs that improved TP (the combined net effect should be around -0.5%).