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.
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
Stop using
CLS_VAR
for "boxed statics" #63845Stop using
CLS_VAR
for "boxed statics" #63845Changes from 6 commits
4a784ed
e7ba4cb
92482c5
175b5f1
9b49938
cbbf50b
6edec21
6d606a2
aa66344
06ca390
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we check the non-faulting flags more generally here instead?
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.
For the common cases the non-faultness is already checked by
OperMayThrow
infgValueNumberAddExceptionSet
.The general form of this check is
IsKnownNonNull(vnpBaseNorm.GetLiberal/Conservative)
, but it leads to many regressions (as well as some improvements).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.
Why does it cause additional regressions?
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.
There is something in how that particular check enables new CSEs that seems to hit weak points in the heuristic and/or downstream allocation. I. e. we get more CSEs, but a lot of them are not profitable.
For reference, here are some of the diffs for the
IsKnownNotNull
change:Win-x64 diffs
Benchmarks, code size:
Benchmarks, PerfScore:
Libraries (PMI), code size:
Libraries (PMI), PerfScore:
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.
Do the diffs generally look bad? Since perf score is better we might consider taking it anyway. The size increase is not that significant to me.
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.
I've analyzed top (~12) PerfScore regressions in the benchmarks on Win-x64, nothing stands out as "definitely bad", the common theme is that we save an additional register (in prolog/epilog) to enregister a CSE. There was one case where a CSE was "live across a call", thus spilled/reloaded for no benefit, but we didn't quite know that in the heuristic because the call in question was a write barrier.
I then took a more cursory look at the tests collection on x86, and found a few quite questionable cases (the worst), though overall the size diff was positive (
-100K
).Finally, I took a look at Win-x64 tests collection, and found a problem that is similar to the one above: diff, where we save/restore (a lot) for a simple (FP) CSE.
Win-Arm64 collections do not appear to have particularly bad examples.
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.
It sounds to me like taking the full general check is fine then, percentage wise the perfscore vs size diff is leaning to the good side, and the bad examples look to be in artificial test cases.