Make idSmallCns signed to allow including -1 #87373
Merged
Conversation
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
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis makes
|
|
There is quite a heavy weight towards BeforeAfter |
tannergooding
changed the title
tannergooding
changed the title
tannergooding
force-pushed
the
xarch-fitsInSmallCns
branch
from
9604d4e to
645fb49
Compare
|
CC. @dotnet/jit-contrib. This is ready for review. As per the summary above, this updates small constants to be tracked as signed which reduces the total emitter allocations for This also cleaned up the There are no diffs. There are some TP diffs ranging from minor improvements to minor regressions which depend on the exact number of bits being used and the required codegen to sign-extend the value, rather than zero extend (this is cheap, but it is an extra instruction). This was done as part of investigating #87016. Not having this makes resolving #87016 much more complex and would cause it to introduce TP regressions. This is because we would have to introduce additional checks/branches to handle the combinations of new EVEX bits alongside the different sized constants (even though SIMD only ever needs 8-bits). Such logic would happen as part of allocating the With this, instead we can rely on SIMD instructions always having a small constant. This then allows us to also make use of bits that are otherwise unused to make it very pay for play as the handling is entirely relegated to the EVEX code paths and only has to be touched on the path that adds the EVEX prefix. |
BruceForstall
approved these changes
Jun 19, 2023
Co-authored-by: Bruce Forstall <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
This makes
idSmallCnssigned on all platforms to allow including -1. This also means that all 8-bit values (signed or unsigned) can currently be tracked as small constants across all platforms. Doing this covers an additional 2% of constants, bringing the total number of small constants up to 94.33%The below comment gives a comparison of the
beforevsafterfor crossgen ofS.P.Corelib. There are various bits of interesting information that can be extrapolated, but the clearest is that 92% of all constants fit into anint8_t. There are then 4.5% of values that will never fit into a small constant (they take more than 16 bits) and so there's only about 1.2% of values that could fit into a small constant if we were given between[11, 16]bits to use.This also includes some cleanup to the
EMITTER_STATShandling/printing to ensure that scenarios are all being tracked.