I don't think we need these three.

In general theseDOTNET_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 disableAvx 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 theDOTNET_PreferredVectorBitWidth knob and the correspondingVector###.IsHardwareAccelerated checks, which is how they're expected to detect this in existing code paths.

--@BruceForstall, there should be no issue in removing a config knob like the variousDOTNET_EnableAvx512*_VL entries, right?

We can simply remove them and document that users should utilizeDOTNET_PreferredVectorBitWidth andDOTNET_EnableAvx512* instead? This would allow a consistent user story here and reduce the overall complexity we need to support/consider

I don't see any problem with removing theDOTNET_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.

Thanks for the feedback.

Please correct me if I am wrong, so we want to removeDOTNET_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 theVL vars, do we want to handle it in this PR, or separately?

so we want to

Yep!

Which I suppose won't be covered in this PR

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 seeAvx10v1.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).

As for the VL vars, do we want to handle it in this PR, or separately?

I'll cover it in a separate PR.

These should behex and the last one should be8, not5.

public const int Avx10v1_v512 = 0x80000000;

This line will raise an error that 0x80000000 cannot be implicitly converted toint, 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.

Eitherpublic const int Avx10v1_v512 = -2147483648; orpublic const int Avx10v1_v512 = unchecked((int)0x80000000)); should be fine

Changing it touint 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.

Changing it to uint or ulong 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.

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.

I'm not sure we need these.

The main intent of these paths is to addoptimisticInstructionSet that is ones where a cached CPUID check is beneficial for AOT code. I don't think that's worth doing forAvx10v1 here since it would only be applicable ifAvx512F was there and that itself requiresVL support.

TheDebug.Assert are then there to setup the expectation thatAvx512F was only supported whenBW+CD+DQ+VL were also supported, even though they are technically supersets ofAvx512F. We don't have a similar consideration here so noDebug.Assert is needed

nit: We should explicitly checkEBX[16] is set to indicateAVX10/V128. The bit exists, so we should be explicit about checking it.

nit: These can only be considered ifAVX10/V128 is set and if theversion was greater than1, so we should have them appropriately nested

nit: None of the other checks use aconst int like this, they inline the(1 << 17) into the if statement and then add a comment indicating what bit it was checking

I can make it to the commonly used form, it looks unnecessary to define this way as the bit-shift form should be easy enough to read in this case.

Doesn't this also need&& CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_EnableAVX10v1)?

And the Avx10v1_V512 case below?

That is, shouldn't all the AVX10 instruction sets be subject to the same configuration flag? (Since you didn't add separate flags for each)

Please see thediscussion here.

We intended to let the implication thatAvx10v1_V512 guaranteesAvx10v1_V256, andAvx10v1_V256 guaranteesAvx10v1 to handle the invalid case (The implication is confirmed by the Avx10 spec doc.). Say, the functionEnusreValidInstructionSetSupport would cancel the illegal flag combination set during the flow.

I can put some comments there if needed.

Comments would be useful.

So on an AVX10 + V512 machine, withDOTNET_EnableAVX10v1=0, we'll leave this code with:

InstructionSet_AVX10v1_V256InstructionSet_AVX10v1_V512

set butInstructionSet_AVX10v1 unset.

Then, we callCPUCompileFlags.EnsureValidInstructionSetSupport(); to "clean it up" in EnsureInstructionSetFlagsAreValid` which has this new code:

        if (resultflags.HasInstructionSet(InstructionSet_AVX10v1_V512) && !resultflags.HasInstructionSet(InstructionSet_AVX10v1_V256))            resultflags.RemoveInstructionSet(InstructionSet_AVX10v1_V512);        if (resultflags.HasInstructionSet(InstructionSet_AVX10v1_V256) && !resultflags.HasInstructionSet(InstructionSet_AVX10v1))            resultflags.RemoveInstructionSet(InstructionSet_AVX10v1_V256);        if (resultflags.HasInstructionSet(InstructionSet_AVX10v1) && !resultflags.HasInstructionSet(InstructionSet_AVX2))            resultflags.RemoveInstructionSet(InstructionSet_AVX10v1);

The first case won't hit, but the others will. That seems to mean thatInstructionSet_AVX10v1_V512 will not be disabled.

If the code ran in a different order, it would work (i.e., check from deepest in the dependency tree first).

I see, thanks for pointing out, in that case, if we want to keep the idea that only checkEnableAvx10v1 once, I can makeAvx10v1_V512 impliesAvx10v1 andAvx10_V256, so there will be an extra check like:

        if (resultflags.HasInstructionSet(InstructionSet_AVX10v1_V512) && !resultflags.HasInstructionSet(InstructionSet_AVX10v1))            resultflags.RemoveInstructionSet(InstructionSet_AVX10v1_V512);

this should be able to avoid the described case.

I presume changing the order for the checks could fix the issue but might not be a solid solution, as if the order changes somehow in the future, this "implicit" bug could be hard to find.

I guess it makes sense to add:

implication        ,X86   ,AVX10v1_V512         ,AVX10v1

and the auto-generated code will do the rest?

Seems like either implication handling could be improved to handle transitive implications better, or, implications are not expected to be transitive. It would be helpful if the documentation/comments made it clear.

I agree, I will document the conclusion in the code.

I think the issue may just be in the order the implications are listed in the doc. We typically want the newest ISAs listed at the bottom