Add empty X64/Arm64 nested classes to some System.Runtime.Intrinsics

[EgorBo]

Rationale

Imagine you have an arm64 CPU with ArmBase and ArmBase.Arm64 support (both are always enabled for any arm64 CPU), but without Aes, Sha1 and Sha256 features, let's run the following snippet:

Console.WriteLine(ArmBase.IsSupported);       // true
Console.WriteLine(ArmBase.Arm64.IsSupported); // true

Console.WriteLine(Aes.IsSupported);           // false
Console.WriteLine(Aes.Arm64.IsSupported);     // true (??)

Console.WriteLine(Sha1.IsSupported);          // false
Console.WriteLine(Sha1.Arm64.IsSupported);    // true (??)

Console.WriteLine(Sha256.IsSupported);        // false
Console.WriteLine(Sha256.Arm64.IsSupported);  // true (??)

The problem here is the fact that Aes, Sha1 and Sha256 inherit from ArmBase but don't define their own Arm64 nested classes, see sharplab.io for more details (it explains why it can't be fixed in the jit).

Proposed API

    [Intrinsic]
    [CLSCompliant(false)]
    public abstract class Sha1 : ArmBase
    {
        internal Sha1() { }

        public static new bool IsSupported { get => IsSupported; }
        
+       public abstract class Arm64 : ArmBase.Arm64;
+       {
+           public static new bool IsSupported => Sha1.IsSupported; // && IntPtr.Size == 8 ?
+       }

(same for Sha256 and Aes)
UPD same for Aes, Avx, Avx2, Fma, Pclmulqdq, Sse3, Ssse3

/cc @tannergooding @echesakovMSFT

[ghost]

Tagging @tannergooding as an area owner

[EgorBo]

Ah, looks like the same problem exists for x64 as well, e.g.:

Console.WriteLine(Avx2.X64.IsSupported);

Prints true on a CPU without AVX2. (because it actually uses Sse42.X64.IsSupported for that)

So we need to do the same for: Aes, Avx, Avx2, Fma, Pclmulqdq, Sse3, Ssse3

[tannergooding]

For x64 this is a possible breaking change if the user recompiles. I still think it provides a better experience, but it is something that needs to be considered.

[terrajobst]

Video

• The problem makes sense, but instead of giving IsSupported behavior, it might be better to hide the nested classes entirely and make calling IsSupported a compile-time error:

    partial class Sha1 : ArmBase
    {
        [EditorBrowsable(Never)]
        public abstract class Arm64            
        {
            // No inheritance, no IsSupported
        }
    }

[tannergooding]

I believe this needs to go back to API review. For x64 in particular, the above doesn't work as there are scenarios like: Sse2 -> Sse3 -> Ssse3 -> Sse41

In this case, Sse2.X64 defines an X64 class as it has new 64-bit specific intrinsics as does Sse41.X64.
Sse3 and Ssse3 do not expose new 64-bit specific intrinsics however.

If we expose intermediate X64 classes which don't inherit and which don't expose an IsSupported then you get into a weird scenario with the actual inheritance hierarchy.
I think the original proposal, which is to expose the new class, have it inherit from BaseIsa.X64 and exppose an IsSupported property is the way to go and will be the most consistent with how the IsSupported checks already work.

It is already the case that Isa.IsSupported can return false but Isa.Method() succeeds because Isa.Method is actually resolved to SomeBaseIsa.Method where SomeBaseIsa.IsSupported is true.

[tannergooding]

CC. @echesakovMSFT, @terrajobst

[terrajobst]

Video

• Good point
• When inheriting, we always should also define all nested types and define IsSupported the appropriately, even if they don't define any other members.
• (this will probably require more nested types than listed below)

[Intrinsic]
[CLSCompliant(false)]
public partial class Sha1 : ArmBase
{
    public abstract class Arm64 : ArmBase.Arm64;
    {
        public static new bool IsSupported => Sha1.IsSupported && IntPtr.Size == 8;
    }
}

[ericstj]

@tannergooding can we add a breaking change doc here? https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

[tannergooding]

dotnet/docs#19682