Fixing the InstructionSetDesc implications

[tannergooding]

Extracted from #85551

This primarily handles ensuring the InstructionSetDesc implications are correct. In particular it ensures that Avx512F is dependent on both AVX2 and FMA. It likewise ensures that the nested VL classes are dependent on both the base VL class and the containing class (e.g. Avx512BW.VL is dependent on both Avx512BW and Avx512F.BW).

It also adds a recursive implication that ensures that F + BW + CD + DQ + VL are only ever enabled as a set. This is done to simplify the current implementation of the JIT without tying us to that behavior permanently. It would be non-trivial work and not "pay for play" to allow this to be represented as a single R2R flag instead.

Finally, as part of fixing this there were a couple minor bugs surfaced. In particular, avx-vnni was marked as opportunistic which in turn marked avx2 as opportunistic. This is problematic if the user decides to target avx but not avx2 and was causing Vector<T> and Vector256<T> to behave incorrectly. This was handled by ensuring avx-vnni is only marked as opportunistic if avx2 is explicitly supported and ensuring that the NI_IsSupported_Dynamic is only returned for opportunistic APIs.

Tests were added covering NAOT, CG2, and JIT validating that the full set of currently exposed ISAs behave correctly. This includes when users explicitly opt into or out-of a given ISA.

[tannergooding]

CC. @jkotas, @davidwrighton

This is the last bit to break apart from #85551 and will leave that as just the Vector<T> changes.

[jkotas]

@davidwrighton Could you please review these new implications?

[davidwrighton]

These implications don't entirely make sense to me. What they indicate is that if any of avx512cd, bw, f, bmi, or the vl variants are enabled, then all the various instruction sets will be considered to be enabled. If that's the case, then why do we need so many flags? I don't understand. In addition, if you're making the implications have this circular flow that isn't actually required by the flags the processor specifies, we need to treat them similarly in the avx512 detection logic in codeman.cpp. Thus, we can't enable ANY of the AVX512 support in the runtime unless all of the associated cpu flags are enabled.

[tannergooding]

What they indicate is that if any of avx512cd, bw, f, bmi, or the vl variants are enabled, then all the various instruction sets will be considered to be enabled

Yes and the comment is meant to explain that, if you have any ideas on how to reword it then please feel free to suggest them.

If that's the case, then why do we need so many flags

We have an actual split in CPUID checks and in other corresponding logic that ties class names to flag names. We may also want to actually allow splitting this in the future, such as if we decide to support Knight's Landing or if some future hardware decides to only support a subset of AVX512.

In addition, if you're making the implications have this circular flow that isn't actually required by the flags the processor specifies, we need to treat them similarly in the avx512 detection logic in codeman.cpp

This is already handled in codeman by virtue of the:

    CPUCompileFlags.Set64BitInstructionSetVariants();
    CPUCompileFlags.EnsureValidInstructionSetSupport();

If Avx512F was supported but Avx512BW was not, then Avx512F would be turned off by the EnsureValidInstructionSetSupport call.

[MichalStrehovsky]

The NativeAOT smoke test change looks good. Thanks for extending the coverage!

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Extracted from #85551

Author:	tannergooding
Assignees:	tannergooding
Labels:	area-CodeGen-coreclr, arch-avx512, needs-area-label
Milestone:	-

[davidwrighton]

I need a better explanation for what we're doing here. See my comments.

[davidwrighton]

What is this for?

[tannergooding]

Avoiding having avx2 be opportunistic because there are some potentially deeper problems and involvement with how it impacts Vector<T>, as you called out on the other PR.

AvxVnni inherits Avx2 and so it being opportunistic means that Avx2 is also opportunistic. This shipped that way in .NET 7 and so its possible there is some R2R issue there.

[davidwrighton]

Could you explain what change is happening here? I don't see a description of what we're changing around NativeAOT behavior in the change description.

[tannergooding]

There was an existing bug here that was surfaced if crossgen/naot targeted avx but not avx2

For most cases, the ISA initially checked and tracked as part of isIsaSupported is the same as what is tracked by the InstructionSetDesc

However, for Vector256 in particular we have the case where the implication is on Avx and we will accelerate some APIs when only Avx is supported. But, we only want IsHardwareAccelerated to report true when Avx2 is also supported.

We were then ending up in a scenario where we'd end up failing to handle IsHardwareAccelerated for the recursive case when avx was supported but avx2 was not because isIsaSupported (AVX) would be true and then we'd fail the compExactlyDependsOn check for AVX2 and then return NI_IsSupported_Dynamic, which was incorrect since avx2 was not opportunistic.

This fixes that so we now ensure that we only go down the true/dynamic path if the compiler could support AVX2 at all.

[davidwrighton]

These implications don't entirely make sense to me. What they indicate is that if any of avx512cd, bw, f, bmi, or the vl variants are enabled, then all the various instruction sets will be considered to be enabled. If that's the case, then why do we need so many flags? I don't understand. In addition, if you're making the implications have this circular flow that isn't actually required by the flags the processor specifies, we need to treat them similarly in the avx512 detection logic in codeman.cpp. Thus, we can't enable ANY of the AVX512 support in the runtime unless all of the associated cpu flags are enabled.