-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: Optimize aggregate info iteration in physical promotion #87997
JIT: Optimize aggregate info iteration in physical promotion #87997
Conversation
In several places we have to iterate over all promoted aggregates. This was less efficient than it has to be because we stored aggregate information in a flat array indexed by local number, and the vast majority of locals are not physically promoted. This change introduces an AggregateInfoMap that stores just the promoted aggregates, and additionally stores an array mapping locals to the promoted aggregates. This does mean that we need a two level lookup to map a local number back to its aggregate information, but that still seems better for throughput than the previous behavior.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIn several places we have to iterate over all promoted aggregates. This was less efficient than it has to be because we stored aggregate information in a flat array indexed by local number, and the vast majority of locals are not physically promoted. This change introduces an AggregateInfoMap that stores just the promoted aggregates, and additionally stores an array mapping locals to the promoted aggregates. This does mean that we need a two level lookup to map a local number back to its aggregate information, but that still seems better for throughput than the previous behavior.
|
// TODO: We need a scalability limit on these, we cannot always track | ||
// the remainder and all fields. |
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.
This was fixed in #87729 by limiting the total number of promotions we make.
access->Count++; | ||
access->CountWtd += weight; | ||
INDEBUG(access->Count++); |
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.
Missed this one in #87969. It would cause checked/release diffs.
cc @dotnet/jit-contrib PTAL @EgorBo Diffs with physical promotion. ASM diffs due to what I mentioned above, where we now insert some readbacks/writebacks in a different order. Other than that just some decent TP improvements. |
// Pointer to the aggregate information, or nullptr if the local is not | ||
// physically promoted. | ||
// | ||
AggregateInfo* AggregateInfoMap::Lookup(unsigned lclNum) |
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.
When I saw Lookup I thought that your map of aggregates is a hash set 🙂
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.
nice tp wins!
In several places we have to iterate over all promoted aggregates. This was less efficient than it has to be because we stored aggregate information in a flat array indexed by local number, and the vast majority of locals are not physically promoted. This change introduces an AggregateInfoMap that stores just the promoted aggregates, and additionally stores an array mapping locals to the promoted aggregates.
This does mean that we need a two level lookup to map a local number back to its aggregate information, but that still seems better for throughput than the previous behavior.
A few minor diffs expected since this means new induced promotions are added at the end of the list, which can cause different ordering when writebacks for them are inserted.