-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Changes from 6 commits
b9b3c8f
619fd39
15d9a47
8e2f33f
f269098
de0a52a
cc876a0
017e73c
fdeb56f
7a18c1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,6 +224,30 @@ public static InstructionSetSupport ConfigureInstructionSetSupport(string instru | |
optimisticInstructionSetSupportBuilder.AddSupportedInstructionSet("avx512vbmi"); | ||
optimisticInstructionSetSupportBuilder.AddSupportedInstructionSet("avx512vbmi_vl"); | ||
} | ||
|
||
Debug.Assert(InstructionSet.X64_AVX10v1_V256 == InstructionSet.X86_AVX10v1_V256); | ||
if (supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX10v1_V256)) | ||
{ | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX10v1)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX2)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_FMA)); | ||
} | ||
|
||
Debug.Assert(InstructionSet.X64_AVX10v1_V512 == InstructionSet.X86_AVX10v1_V512); | ||
if (supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX10v1_V512)) | ||
{ | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX10v1_V256)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512F)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512F_VL)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512BW)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512BW_VL)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512CD)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512CD_VL)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512DQ)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512DQ_VL)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512VBMI)); | ||
Debug.Assert(supportedInstructionSet.HasInstructionSet(InstructionSet.X64_AVX512VBMI_VL)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we need these. The main intent of these paths is to add The |
||
} | ||
else if (targetArchitecture == TargetArchitecture.ARM64) | ||
{ | ||
|
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.
These should be
hex
and the last one should be8
, not5
.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.
public const int Avx10v1_v512 = 0x80000000;
This line will raise an error that 0x80000000 cannot be implicitly converted to
int
, although it can be fixed by explicitly defining as:public const int Avx10v1_v512 = -2147483648;
, I wonder if this is a proper fix (I suppose it should be good in terms of the number itself as it is just a flag bit), considering it has reached the max number of ISA this design can hold, and we are expecting some more ISAs coming in the future, e.g. higher version of Avx10 and APX, etc.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.
Either
public const int Avx10v1_v512 = -2147483648;
orpublic const int Avx10v1_v512 = unchecked((int)0x80000000));
should be fineChanging it to
uint
orulong
to hold more bits might also be appropriate, it's probably a better question for @MichalStrehovsky on how he wants it handled long term for this part of the codebase.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 have an opinion; whatever works. At this pace we may run out of ulong bits too. I don't have a good understanding for why we need so many bits: the VL suffixes look redundant; the various VectorT are also redundant with other flags we set at the same time, etc.
I was involved in the design for this up to AVX2/BMI but haven't really though about it since. The old design doesn't scale if we use bits at this pace because we can't make SSE3/SSE41/etc. baseline yet (and free up bits that way), but we may already need new bits. It would be better for someone who actually understands this to design a shape that will work.
There is an advantage if this can fit in 32bits (run-time check when IsSupported is done dynamically at runtime use the same enum and 32bits are simpler), but we can also introduce yet another enum for those purposes that doesn't waste bits, or have two ints, or something like that. If we can reclaim those 7 bits, it might be enough breathing room to keep using this enum and 32bits.