Updating Vector*.IsHardwareAccelerated to be recursive (#69578)

* Updating Vector*.IsHardwareAccelerated to be recursive so it works in the debugger * Adding tests validating indirect and direct invocation of get_IsHardwareAccelerated return the same value * Ensure that Vector<T>.IsHardwareAccelerated supports being recursive * Apply suggestions from code review Co-authored-by: Stephen Toub <stoub@microsoft.com> * Update src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs * Detect the one legal violation of the behaves the same rule for intrinsics in CoreLib - The functions which detect if an intrinsic is actually available are allowed to have behavior which differs based on which intrinsics are available at runtime * [mono] Intrinsify IsHardwareAccelerated in the interpreter. * Add a fallback path for unhandled recursive intrinsics in System.Numerics * Move the Vector.IsHardwareAccelerated handling into SimdAsHWIntrinsicInfo.lookupId * Fixinng a compiler error due to an unresolved member * Adjust the NI_IsSupported_Dynamic checks to be NAOT only Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: David Wrighton <davidwr@microsoft.com> Co-authored-by: Zoltan Varga <vargaz@gmail.com>
dotnet · Sep 17, 2022 · b40c3a3 · b40c3a3
1 parent f45df0d
commit b40c3a3
Show file tree

Hide file tree

Showing 15 changed files with 111 additions and 13 deletions.
diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -1830,6 +1830,7 @@ class Compiler
 
 #ifdef FEATURE_HW_INTRINSICS
     friend struct HWIntrinsicInfo;
+    friend struct SimdAsHWIntrinsicInfo;
 #endif // FEATURE_HW_INTRINSICS
 
 #ifndef TARGET_64BIT

diff --git a/src/coreclr/jit/hwintrinsic.cpp b/src/coreclr/jit/hwintrinsic.cpp
@@ -314,8 +314,38 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler*         comp,
 
     if ((strcmp(methodName, "get_IsSupported") == 0) || isHardwareAcceleratedProp)
     {
-        return isIsaSupported ? (comp->compExactlyDependsOn(isa) ? NI_IsSupported_True : NI_IsSupported_Dynamic)
-                              : NI_IsSupported_False;
+        // The `compSupportsHWIntrinsic` above validates `compSupportsIsa` indicating
+        // that the compiler can emit instructions for the ISA but not whether the
+        // hardware supports them.
+        //
+        // The `compExactlyDependsOn` on call then validates that the target hardware
+        // supports the instruction. Normally this is the same ISA as we just checked
+        // but for Vector128/256 on xarch this can be a different ISA since we accelerate
+        // some APIs even when we can't accelerate all APIs.
+        //
+        // When the target hardware does support the instruction set, we can return a
+        // constant true. When it doesn't then we want to report the check as dynamically
+        // supported instead. This allows some targets, such as AOT, to emit a check against
+        // a cached CPU query so lightup can still happen (such as for SSE4.1 when the target
+        // hardware is SSE2).
+        //
+        // When the compiler doesn't support ISA or when it does but the target hardware does
+        // not and we aren't in a scenario with support for a dynamic check, we want to return false.
+
+        if (isIsaSupported)
+        {
+            if (comp->compExactlyDependsOn(isa))
+            {
+                return NI_IsSupported_True;
+            }
+
+            if (comp->IsTargetAbi(CORINFO_NATIVEAOT_ABI))
+            {
+                return NI_IsSupported_Dynamic;
+            }
+        }
+
+        return NI_IsSupported_False;
     }
     else if (!isIsaSupported)
     {

diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp
@@ -5960,17 +5960,36 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
             result = NI_System_Numerics_BitOperations_PopCount;
         }
     }
-#ifdef FEATURE_HW_INTRINSICS
     else if (strcmp(namespaceName, "System.Numerics") == 0)
     {
+#ifdef FEATURE_HW_INTRINSICS
         CORINFO_SIG_INFO sig;
         info.compCompHnd->getMethodSig(method, &sig);
 
         int sizeOfVectorT = getSIMDVectorRegisterByteLength();
 
-        result = SimdAsHWIntrinsicInfo::lookupId(&sig, className, methodName, enclosingClassName, sizeOfVectorT);
-    }
+        result = SimdAsHWIntrinsicInfo::lookupId(this, &sig, className, methodName, enclosingClassName, sizeOfVectorT);
 #endif // FEATURE_HW_INTRINSICS
+
+        if (result == NI_Illegal)
+        {
+            if (strcmp(methodName, "get_IsHardwareAccelerated") == 0)
+            {
+                // This allows the relevant code paths to be dropped as dead code even
+                // on platforms where FEATURE_HW_INTRINSICS is not supported.
+
+                result = NI_IsSupported_False;
+            }
+            else if (gtIsRecursiveCall(method))
+            {
+                // For the framework itself, any recursive intrinsics will either be
+                // only supported on a single platform or will be guarded by a relevant
+                // IsSupported check so the throw PNSE will be valid or dropped.
+
+                result = NI_Throw_PlatformNotSupportedException;
+            }
+        }
+    }
     else if (strcmp(namespaceName, "System.Runtime.CompilerServices") == 0)
     {
         if (strcmp(className, "Unsafe") == 0)

diff --git a/src/coreclr/jit/simdashwintrinsic.cpp b/src/coreclr/jit/simdashwintrinsic.cpp
@@ -51,7 +51,8 @@ const SimdAsHWIntrinsicInfo& SimdAsHWIntrinsicInfo::lookup(NamedIntrinsic id)
 //
 // Return Value:
 //    The NamedIntrinsic associated with methodName and classId
-NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(CORINFO_SIG_INFO* sig,
+NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(Compiler*         comp,
+                                               CORINFO_SIG_INFO* sig,
                                                const char*       className,
                                                const char*       methodName,
                                                const char*       enclosingClassName,
@@ -73,6 +74,11 @@ NamedIntrinsic SimdAsHWIntrinsicInfo::lookupId(CORINFO_SIG_INFO* sig,
         isInstanceMethod = true;
     }
 
+    if (strcmp(methodName, "get_IsHardwareAccelerated") == 0)
+    {
+        return comp->IsBaselineSimdIsaSupported() ? NI_IsSupported_True : NI_IsSupported_False;
+    }
+
     for (int i = 0; i < (NI_SIMD_AS_HWINTRINSIC_END - NI_SIMD_AS_HWINTRINSIC_START - 1); i++)
     {
         const SimdAsHWIntrinsicInfo& intrinsicInfo = simdAsHWIntrinsicInfoArray[i];

diff --git a/src/coreclr/jit/simdashwintrinsic.h b/src/coreclr/jit/simdashwintrinsic.h
@@ -68,7 +68,8 @@ struct SimdAsHWIntrinsicInfo
 
     static const SimdAsHWIntrinsicInfo& lookup(NamedIntrinsic id);
 
-    static NamedIntrinsic lookupId(CORINFO_SIG_INFO* sig,
+    static NamedIntrinsic lookupId(Compiler*         comp,
+                                   CORINFO_SIG_INFO* sig,
                                    const char*       className,
                                    const char*       methodName,
                                    const char*       enclosingClassName,

diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
@@ -3924,8 +3924,11 @@ private bool notifyInstructionSetUsage(InstructionSet instructionSet, bool suppo
             else
             {
                 // By policy we code review all changes into corelib, such that failing to use an instruction
-                // set is not a reason to not support usage of it.
-                if (!isMethodDefinedInCoreLib())
+                // set is not a reason to not support usage of it. Except for functions which check if a given
+                // feature is supported or hardware accelerated.
+                if (!isMethodDefinedInCoreLib() ||
+                    MethodBeingCompiled.Name == "get_IsSupported" ||
+                    MethodBeingCompiled.Name == "get_IsHardwareAccelerated")
                 {
                     _actualInstructionSetUnsupported.AddInstructionSet(instructionSet);
                 }

diff --git a/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs b/src/libraries/System.Numerics.Vectors/tests/GenericVectorTests.cs
@@ -25,6 +25,13 @@ static GenericVectorTests()
             dummy = System.Numerics.Vector<float>.One;
         }
 
+        [Fact]
+        public unsafe void IsHardwareAcceleratedTest()
+        {
+            MethodInfo methodInfo = typeof(Vector).GetMethod("get_IsHardwareAccelerated");
+            Assert.Equal(Vector.IsHardwareAccelerated, methodInfo.Invoke(null, null));
+        }
+
         #region Constructor Tests
 
         [Fact]

diff --git a/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs b/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs
@@ -18,7 +18,7 @@ public static partial class Vector
         public static bool IsHardwareAccelerated
         {
             [Intrinsic]
-            get => false;
+            get => IsHardwareAccelerated;
         }
 
         /// <summary>Computes the absolute value of each element in a vector.</summary>

diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128.cs
@@ -44,7 +44,7 @@ public static class Vector128
         public static bool IsHardwareAccelerated
         {
             [Intrinsic]
-            get => false;
+            get => IsHardwareAccelerated;
         }
 
         /// <summary>Computes the absolute value of each element in a vector.</summary>

diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector256.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector256.cs
@@ -45,7 +45,7 @@ public static class Vector256
         public static bool IsHardwareAccelerated
         {
             [Intrinsic]
-            get => false;
+            get => IsHardwareAccelerated;
         }
 
         /// <summary>Computes the absolute value of each element in a vector.</summary>

diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector64.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector64.cs
@@ -21,7 +21,7 @@ public static class Vector64
         public static bool IsHardwareAccelerated
         {
             [Intrinsic]
-            get => false;
+            get => IsHardwareAccelerated;
         }
 
         /// <summary>Computes the absolute value of each element in a vector.</summary>

diff --git a/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector128Tests.cs b/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector128Tests.cs
@@ -9,6 +9,13 @@ namespace System.Runtime.Intrinsics.Tests.Vectors
 {
     public sealed class Vector128Tests
     {
+        [Fact]
+        public unsafe void Vector128IsHardwareAcceleratedTest()
+        {
+            MethodInfo methodInfo = typeof(Vector128).GetMethod("get_IsHardwareAccelerated");
+            Assert.Equal(Vector128.IsHardwareAccelerated, methodInfo.Invoke(null, null));
+        }
+
         [Fact]
         public unsafe void Vector128ByteExtractMostSignificantBitsTest()
         {

diff --git a/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector256Tests.cs b/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector256Tests.cs
@@ -9,6 +9,13 @@ namespace System.Runtime.Intrinsics.Tests.Vectors
 {
     public sealed class Vector256Tests
     {
+        [Fact]
+        public unsafe void Vector256IsHardwareAcceleratedTest()
+        {
+            MethodInfo methodInfo = typeof(Vector256).GetMethod("get_IsHardwareAccelerated");
+            Assert.Equal(Vector256.IsHardwareAccelerated, methodInfo.Invoke(null, null));
+        }
+
         [Fact]
         public unsafe void Vector256ByteExtractMostSignificantBitsTest()
         {

diff --git a/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs b/src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs
@@ -10,6 +10,13 @@ namespace System.Runtime.Intrinsics.Tests.Vectors
 {
     public sealed class Vector64Tests
     {
+        [Fact]
+        public unsafe void Vector64IsHardwareAcceleratedTest()
+        {
+            MethodInfo methodInfo = typeof(Vector64).GetMethod("get_IsHardwareAccelerated");
+            Assert.Equal(Vector64.IsHardwareAccelerated, methodInfo.Invoke(null, null));
+        }
+
         [Fact]
         public unsafe void Vector64ByteExtractMostSignificantBitsTest()
         {

diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c
@@ -2470,6 +2470,16 @@ interp_handle_intrinsics (TransformData *td, MonoMethod *target_method, MonoClas
 		(!strncmp ("System.Runtime.Intrinsics.Arm", klass_name_space, 29) ||
 		!strncmp ("System.Runtime.Intrinsics.X86", klass_name_space, 29))) {
 		interp_generate_void_throw (td, MONO_JIT_ICALL_mono_throw_platform_not_supported);
+	} else if (in_corlib &&
+			   (!strncmp ("System.Numerics", klass_name_space, 15) &&
+				!strcmp ("Vector", klass_name) &&
+				!strcmp (tm, "get_IsHardwareAccelerated"))) {
+		*op = MINT_LDC_I4_0;
+	} else if (in_corlib &&
+			   (!strncmp ("System.Runtime.Intrinsics", klass_name_space, 25) &&
+				!strncmp ("Vector", klass_name, 6) &&
+				!strcmp (tm, "get_IsHardwareAccelerated"))) {
+		*op = MINT_LDC_I4_0;
 	}
 
 	return FALSE;