-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Adding Avx10v1
to the runtime
#99784
Merged
Merged
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
b9b3c8f
script-gen changes
Ruihan-Yin 619fd39
hand-written changes
Ruihan-Yin 15d9a47
improve the CPUID check logic.
Ruihan-Yin 8e2f33f
Add missing changes in managed code and fix bad naming.
Ruihan-Yin f269098
Resolve comments:
Ruihan-Yin de0a52a
Add missing definition and implication in ISA descriptor.
Ruihan-Yin cc876a0
Resolve comments:
Ruihan-Yin 017e73c
Resolve comment:
Ruihan-Yin fdeb56f
Merge branch 'main' into Avx103
Ruihan-Yin 7a18c1c
Resolve comments:
Ruihan-Yin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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 don't think we need these three.
In general these
DOTNET_Enable{Isa}
knobs are primarily there for testing purposes. That is, they exist so that developers with newer hardware can still test downlevel code paths without having to recompile their applications. Such code is not typically meant for use in actual production scenarios.To that end, we typically expose one knob per CPUID ISA bit and so you can disable
Avx
but notAvx.X64
.Avx512F.VL
was somewhat an exception and in hindsight, probably a knob we don't actually need to let users control. I expect that theV256
andV512
nested classes fit into the same bucket, where users don't need the ability to enable/disable them directly and independently ofAvx10
itself.Instead, users who need to control access to the used vector bit width can utilize the
DOTNET_PreferredVectorBitWidth
knob and the correspondingVector###.IsHardwareAccelerated
checks, which is how they're expected to detect this in existing code paths.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.
-- @BruceForstall, there should be no issue in removing a config knob like the various
DOTNET_EnableAvx512*_VL
entries, right?We can simply remove them and document that users should utilize
DOTNET_PreferredVectorBitWidth
andDOTNET_EnableAvx512*
instead? This would allow a consistent user story here and reduce the overall complexity we need to support/considerThere 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 don't see any problem with removing the
DOTNET_EnableAvx512*_VL
configs. They are available in Release and have been shipped, and use theEXTERNAL
prefix, which indicates (or at least historically indicated) a documented config. (Maybe we should have used the prefixUNSUPPORTED
?) So technically removing them is a breaking change, but it doesn't seem like a problem.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.
Thanks for the feedback.
Please correct me if I am wrong, so we want to remove
DOTNET_EnableAvx10v1_V256/512
andDOTNET_Avx10MaxVectorLength
. And keepDOTNET_EnableAvx10v1
, let it plusDOTNET_PreferredVectorBitWidth
to control the emit behavior ofVector256/512
APIs. (Which I suppose won't be covered in this PR.)As for the
VL
vars, do we want to handle it in this PR, or separately?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.
Yep!
This should already be somewhat implicit based on the existing checks we have in the JIT, so I don't expect we need to do anything special. Users will still see
Avx10v1.V512
report supported if the hardware actually supports it and they would simply checkVector512.IsHardwareAccelerated
if they don't want limit 512-bit usage to the user selected behavior (which is controlled byPreferredVectorBitWidth
).I'll cover it in a separate PR.