Enable must-throw for unsupported intrinsics under optimizations

src/coreclr/jit/hwintrinsic.cpp

@@ -1204,11 +1204,18 @@ static NamedIntrinsic binarySearchId(CORINFO_InstructionSet isa,
 // lookupId: Gets the NamedIntrinsic for a given method name and InstructionSet
 //
 // Arguments:
-//    comp       -- The compiler
-//    sig        -- The signature of the intrinsic
-//    className  -- The name of the class associated with the HWIntrinsic to lookup
-//    methodName -- The name of the method associated with the HWIntrinsic to lookup
-//    enclosingClassName -- The name of the enclosing class of X64 classes
+//    comp                    -- The compiler
+//    sig                     -- The signature of the intrinsic
+//    className               -- The name of the class associated with the HWIntrinsic to lookup
+//    methodName              -- The name of the method associated with the HWIntrinsic to lookup
+//    innerEnclosingClassName -- The name of the inner enclosing class of nested 64-bit classes
+//    outerEnclosingClassName -- The name of the outer enclosing class of nested 64-bit classes
+//    isXplatIntrinsic        -- True if the intrinsic lives directly under the cross-platform
+//                               System.Runtime.Intrinsics (or System.Numerics) namespace and
+//                               therefore has a managed fallback when the underlying ISA isn't
+//                               available; false if it is platform-specific (under
+//                               System.Runtime.Intrinsics.<Arch>) and must throw a
+//                               PlatformNotSupportedException when the ISA isn't available.
 //
 // Return Value:
 //    The NamedIntrinsic associated with methodName and isa
@@ -1217,7 +1224,8 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler*         comp,
                                          const char*       className,
                                          const char*       methodName,
                                          const char*       innerEnclosingClassName,
-                                         const char*       outerEnclosingClassName)
+                                         const char*       outerEnclosingClassName,
+                                         bool              isXplatIntrinsic)
 {
 #if defined(DEBUG)
     static bool validationCompleted = false;
@@ -1335,7 +1343,24 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler*         comp,
     }
     else if (!isIsaSupported)
     {
-        return NI_Throw_PlatformNotSupportedException;
+        // We only want to surface a hard PlatformNotSupportedException when the ISA is
+        // unsupported AND there is no viable managed fallback. That is only the case for
+        // the platform-specific APIs under System.Runtime.Intrinsics.<Arch> (e.g.,
+        // Avx512.LoadVector512). The cross-platform APIs in System.Runtime.Intrinsics
+        // (e.g., Vector512.Load, Vector128<T>.AsByte) and System.Numerics (e.g.,
+        // Vector<T>) all have a managed fallback that should be used when the underlying
+        // ISA isn't available. For those, we fall through to the per-ISA handling below
+        // which returns NI_Illegal, allowing the call to be treated as a normal managed
+        // method (so the body can be inlined / executed normally).
+
+        if (!isXplatIntrinsic)
+        {
+            return NI_Throw_PlatformNotSupportedException;
+        }
+        else
+        {
+            return NI_Illegal;
+        }
     }
 
     // Special case: For Vector64/128/256 we currently don't accelerate any of the methods when

src/coreclr/jit/hwintrinsic.h

@@ -532,7 +532,8 @@ struct HWIntrinsicInfo
                                    const char*       className,
                                    const char*       methodName,
                                    const char*       innerEnclosingClassName,
-                                   const char*       outerEnclosingClassName);
+                                   const char*       outerEnclosingClassName,
+                                   bool              isXplatIntrinsic);
 
     static unsigned lookupSimdSize(Compiler* comp, NamedIntrinsic id, CORINFO_SIG_INFO* sig);
 

src/coreclr/jit/importercalls.cpp

@@ -3247,7 +3247,7 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE    clsHnd,
 
                         default:
                         {
-                            return impUnsupportedNamedIntrinsic(CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED, method, sig,
+                            return impUnsupportedNamedIntrinsic(CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED, method, sig,
                                                                 mustExpand);
                         }
                     }
@@ -5206,9 +5206,9 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE    clsHnd,
 #ifdef TARGET_WASM
         NYI_WASM("Unhandled must expand intrinsic");
 #else
-        assert(!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException");
+        assert(!"Unhandled must expand intrinsic, throwing NotImplementedException");
 #endif
-        return impUnsupportedNamedIntrinsic(CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED, method, sig, mustExpand);
+        return impUnsupportedNamedIntrinsic(CORINFO_HELP_THROW_NOT_IMPLEMENTED, method, sig, mustExpand);
     }
 
     // Optionally report if this intrinsic is special
@@ -11091,8 +11091,12 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
                                 CORINFO_SIG_INFO sig;
                                 info.compCompHnd->getMethodSig(method, &sig);
 
+                                // System.Numerics.Vector<T> and System.Numerics.Vector are cross-platform
+                                // APIs with managed fallbacks; they must not throw PNSE when the ISA isn't
+                                // available.
                                 result = HWIntrinsicInfo::lookupId(this, &sig, lookupClassName, lookupMethodName,
-                                                                   enclosingClassNames[0], enclosingClassNames[1]);
+                                                                   enclosingClassNames[0], enclosingClassNames[1],
+                                                                   /* isXplatIntrinsic */ true);
                             }
                         }
 #endif // FEATURE_HW_INTRINSICS
@@ -11378,13 +11382,18 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
                             }
                         }
 
-                        if ((namespaceName[0] == '\0') || (strcmp(namespaceName, platformNamespaceName) == 0))
+                        bool isXplatIntrinsic              = (namespaceName[0] == '\0');
+                        bool isPlatformMatchedIntrinsic    = (strcmp(namespaceName, platformNamespaceName) == 0);
+                        bool isPlatformMismatchedIntrinsic = !isXplatIntrinsic && !isPlatformMatchedIntrinsic;
+
+                        if (isXplatIntrinsic || isPlatformMatchedIntrinsic)
                         {
                             CORINFO_SIG_INFO sig;
                             info.compCompHnd->getMethodSig(method, &sig);
 
-                            result = HWIntrinsicInfo::lookupId(this, &sig, className, methodName,
-                                                               enclosingClassNames[0], enclosingClassNames[1]);
+                            result =
+                                HWIntrinsicInfo::lookupId(this, &sig, className, methodName, enclosingClassNames[0],
+                                                          enclosingClassNames[1], isXplatIntrinsic);
                         }
 #endif // FEATURE_HW_INTRINSICS
 
@@ -11429,6 +11438,16 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
 
                                 result = NI_Throw_PlatformNotSupportedException;
                             }
+#ifdef FEATURE_HW_INTRINSICS
+                            else if (isPlatformMismatchedIntrinsic)
+                            {
+                                // The API lives in a platform-specific sub-namespace under
+                                // System.Runtime.Intrinsics (e.g., .X86 on ARM64, .Wasm on xarch) that
+                                // does not match the target architecture. Such APIs are platform-specific
+                                // with no managed fallback, so they must throw PlatformNotSupportedException.
+                                result = NI_Throw_PlatformNotSupportedException;
+                            }
+#endif // FEATURE_HW_INTRINSICS
                             else
                             {
                                 // Otherwise mark this as a general intrinsic in the namespace
@@ -11555,6 +11574,12 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method)
     {
         JITDUMP("Not recognized\n");
     }
+    else if ((result == NI_System_Numerics_Intrinsic) || (result == NI_System_Runtime_Intrinsics_Intrinsic))
+    {
+        // These are special markers used just to ensure we still get the inlining profitability
+        // boost. We actually have the implementation in managed, however, to keep the JIT simpler.
+        JITDUMP("Not recognized - inlining boost\n");
+    }
     else if (result == NI_IsSupported_False)
     {
         JITDUMP("Unsupported - return false");
@@ -11980,7 +12005,7 @@ NamedIntrinsic Compiler::lookupPrimitiveIntNamedIntrinsic(CORINFO_METHOD_HANDLE
 //    mustExpand - true if the intrinsic must return a GenTree*; otherwise, false
 //
 // Return Value:
-//    a gtNewMustThrowException if mustExpand is true; otherwise, nullptr
+//    a gtNewMustThrowException if mustExpand is true or optimizations are enabled; otherwise, nullptr
 //
 GenTree* Compiler::impUnsupportedNamedIntrinsic(unsigned              helper,
                                                 CORINFO_METHOD_HANDLE method,
@@ -11989,18 +12014,36 @@ GenTree* Compiler::impUnsupportedNamedIntrinsic(unsigned              helper,
 {
     // We've hit some error case and may need to return a node for the given error.
     //
-    // When `mustExpand=false`, we are attempting to inline the intrinsic directly into another method. In this
-    // scenario, we need to return `nullptr` so that a GT_CALL to the intrinsic is emitted instead. This is to
-    // ensure that everything continues to behave correctly when optimizations are enabled (e.g. things like the
-    // inliner may expect the node we return to have a certain signature, and the `MustThrowException` node won't
-    // match that).
-    //
     // When `mustExpand=true`, we are in a GT_CALL to the intrinsic and are attempting to JIT it. This will generally
     // be in response to an indirect call (e.g. done via reflection) or in response to an earlier attempt returning
     // `nullptr` (under `mustExpand=false`). In that scenario, we are safe to return the `MustThrowException` node.
+    //
+    // When `mustExpand=false`, we are attempting to expand the intrinsic directly at the call site (either at the
+    // root method or while importing an inlinee body). When optimizations are enabled it is preferable to surface
+    // a hard throw at the unsupported call site rather than fall back to a managed call into the (unsupported)
+    // intrinsic body, since the body itself will only end up throwing as well. We therefore try to also return the
+    // `MustThrowException` node when `opts.OptimizationEnabled()` is true. When optimizations are disabled (Tier0
+    // / MinOpts / debug code) we keep the legacy behavior of returning `nullptr` and emitting a GT_CALL so as not
+    // to perturb debugging or tier-up scenarios.
+
+    if ((helper == CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED) && IsAot())
+    {
+        // However, if we're AOT and must expand, we need to throw a PlatformNotSupportedException since there is
+        // no NAOT/R2R helper for the regular NotSupportedException with the relevant type message. PNSE derives
+        // from NSE and so it is still accurate for most considerations. We can fix this the next time we bump
+        // MINIMUM_READYTORUN_MAJOR_VERSION
+
+        if (!mustExpand)
+        {
+            return nullptr;
+        }
+        helper = CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED;
+    }
 
-    if (mustExpand)
+    if (mustExpand || opts.OptimizationEnabled())
     {
+        impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("impUnsupportedNamedIntrinsic"));
+
         for (unsigned i = 0; i < sig->numArgs; i++)
         {
             impPopStack();