From 0c10cbca3dfd0ab55b943885a34dcb19856a498e Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 17 Jun 2025 15:18:18 -0700 Subject: [PATCH 01/10] Checkpoint Checkpoint Extract named intrinsic lookup out of JIT Compiler class Maybe fix zbb bit Basic wire-up of interpreter intrinsics Bypass more intrinsics Don't assert on unimplemented intrinsic by default, just don't handle it Add more intrinsics to the switch Fix linux build Revert changes to JIT side Checkpoint intrinsics rework Checkpoint Change EH workaround to something with a smaller blast radius Fix linux build Checkpoint: Add DOTNET_InterpMode config var and pipe it through Minor adjustments suggested offline Address PR feedback + maybe fix CI build Rearrange intrinsics test so it won't fail on ARM64 CI --- src/coreclr/interpreter/CMakeLists.txt | 1 + src/coreclr/interpreter/compiler.cpp | 52 ++++++++ src/coreclr/interpreter/compiler.h | 2 + src/coreclr/interpreter/eeinterp.cpp | 60 +++++++-- src/coreclr/interpreter/interpconfigvalues.h | 5 + src/coreclr/interpreter/intops.def | 1 + src/coreclr/interpreter/intrinsics.cpp | 128 +++++++++++++++++++ src/coreclr/interpreter/intrinsics.h | 11 ++ src/coreclr/vm/interpexec.cpp | 5 +- src/tests/JIT/interpreter/Interpreter.cs | 32 +++++ 10 files changed, 282 insertions(+), 15 deletions(-) create mode 100644 src/coreclr/interpreter/intrinsics.cpp create mode 100644 src/coreclr/interpreter/intrinsics.h diff --git a/src/coreclr/interpreter/CMakeLists.txt b/src/coreclr/interpreter/CMakeLists.txt index 06536580d1a440..00536cb13a8286 100644 --- a/src/coreclr/interpreter/CMakeLists.txt +++ b/src/coreclr/interpreter/CMakeLists.txt @@ -12,6 +12,7 @@ set(INTERPRETER_SOURCES stackmap.cpp naming.cpp methodset.cpp + intrinsics.cpp ../../native/containers/dn-simdhash.c ../../native/containers/dn-simdhash-ght-compatible.c ../../native/containers/dn-simdhash-ptr-ptr.c) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 6b49b7bcd3d3a7..445f01180e2283 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2180,6 +2180,42 @@ int32_t InterpCompiler::GetDataItemIndexForHelperFtn(CorInfoHelpFunc ftn) return GetDataItemIndex(addr); } +bool InterpCompiler::EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig) +{ + bool mustExpand = (method == m_methodHnd); + if (!mustExpand && (ni == NI_Illegal)) + return false; + + switch (ni) + { + case NI_IsSupported_False: + case NI_IsSupported_True: + AddIns(INTOP_LDC_I4); + m_pLastNewIns->data[0] = ni == NI_IsSupported_True; + PushStackType(StackTypeI4, NULL); + m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); + return true; + + case NI_Throw_PlatformNotSupportedException: + // Interpreter-FIXME: If an INTOP_THROW_PNSE is the first opcode in a try block, catch doesn't catch the exception + // For now, insert a safepoint as padding. + AddIns(INTOP_SAFEPOINT); + AddIns(INTOP_THROW_PNSE); + return true; + + default: + { + const char* className = NULL; + const char* namespaceName = NULL; + const char* methodName = m_compHnd->getMethodNameFromMetadata(method, &className, &namespaceName, NULL, 0); + printf("WARNING: Intrinsic not implemented in EmitNamedIntrinsicCall: %d (for %s.%s.%s)\n", ni, namespaceName, className, methodName); + if (mustExpand) + assert(!"EmitNamedIntrinsicCall not implemented must-expand intrinsic"); + return false; + } + } +} + bool InterpCompiler::EmitCallIntrinsics(CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig) { const char *className = NULL; @@ -2584,6 +2620,22 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re flags = (CORINFO_CALLINFO_FLAGS)(flags | CORINFO_CALLINFO_CALLVIRT); m_compHnd->getCallInfo(&resolvedCallToken, pConstrainedToken, m_methodInfo->ftn, flags, &callInfo); + if (callInfo.methodFlags & CORINFO_FLG_INTRINSIC) + { +// Interpreter-FIXME: Necessary to work around InterpConfig members only being defined in DEBUG configurations. +#if DEBUG + if (InterpConfig.InterpMode() >= 3) + { + NamedIntrinsic ni = GetNamedIntrinsic(m_compHnd, m_methodHnd, callInfo.hMethod); + if (EmitNamedIntrinsicCall(ni, resolvedCallToken.hClass, callInfo.hMethod, callInfo.sig)) + { + m_ip += 5; + return; + } + } +#endif + } + if (EmitCallIntrinsics(callInfo.hMethod, callInfo.sig)) { m_ip += 5; diff --git a/src/coreclr/interpreter/compiler.h b/src/coreclr/interpreter/compiler.h index 95a04bc2378ade..1ea75c8db439bb 100644 --- a/src/coreclr/interpreter/compiler.h +++ b/src/coreclr/interpreter/compiler.h @@ -11,6 +11,7 @@ #include #include "failures.h" #include "simdhash.h" +#include "intrinsics.h" struct InterpException { @@ -695,6 +696,7 @@ class InterpCompiler void EmitShiftOp(int32_t opBase); void EmitCompareOp(int32_t opBase); void EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool readonly, bool tailcall, bool newObj, bool isCalli); + bool EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig); bool EmitCallIntrinsics(CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO sig); void EmitLdind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset); void EmitStind(InterpType type, CORINFO_CLASS_HANDLE clsHnd, int32_t offset, bool reverseSVarOrder); diff --git a/src/coreclr/interpreter/eeinterp.cpp b/src/coreclr/interpreter/eeinterp.cpp index c449a53714949a..4230cd554e2a73 100644 --- a/src/coreclr/interpreter/eeinterp.cpp +++ b/src/coreclr/interpreter/eeinterp.cpp @@ -48,27 +48,59 @@ CorJitResult CILInterp::compileMethod(ICorJitInfo* compHnd, uint32_t* nativeSizeOfCode) { - bool doInterpret; + bool doInterpret = false; + + if ((g_interpModule != NULL) && (methodInfo->scope == g_interpModule)) + doInterpret = true; - if (g_interpModule != NULL) - { - if (methodInfo->scope == g_interpModule) - doInterpret = true; - else - doInterpret = false; - } - else { - const char *methodName = compHnd->getMethodNameFromMetadata(methodInfo->ftn, nullptr, nullptr, nullptr, 0); +// Interpreter-FIXME: Necessary to work around InterpConfig members only being defined in DEBUG. +#if DEBUG + switch (InterpConfig.InterpMode()) + { + // 0: default, do not use interpreter except explicit opt-in via DOTNET_Interpreter + case 0: + break; + + // 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. This mode should have good balance between speed and coverage. + case 1: + { + doInterpret = true; + const char *assemblyName = compHnd->getClassAssemblyName(compHnd->getMethodClass(methodInfo->ftn)); + if (assemblyName && !strcmp(assemblyName, "System.Private.CoreLib")) + doInterpret = false; + break; + } + + // 2: use interpreter for everything except intrinsics. All intrinsics fallback to JIT. Implies DOTNET_ReadyToRun=0. + case 2: + doInterpret = !(compHnd->getMethodAttribs(methodInfo->ftn) & CORINFO_FLG_INTRINSIC); + break; + + // 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0, + case 3: + doInterpret = true; + break; + + default: + assert(!"Unsupported value for DOTNET_InterpMode"); + break; + } +#endif + #ifdef TARGET_WASM // interpret everything on wasm doInterpret = true; #else - doInterpret = (InterpConfig.Interpreter().contains(compHnd, methodInfo->ftn, compHnd->getMethodClass(methodInfo->ftn), &methodInfo->args)); -#endif - - if (doInterpret) + // NOTE: We do this check even if doInterpret==true in order to populate g_interpModule + const char *methodName = compHnd->getMethodNameFromMetadata(methodInfo->ftn, nullptr, nullptr, nullptr, 0); + if (InterpConfig.Interpreter().contains(compHnd, methodInfo->ftn, compHnd->getMethodClass(methodInfo->ftn), &methodInfo->args)) + { + doInterpret = true; + // FIXME: What if there are multiple modules specified by the DOTNET_Interpreter config variable? g_interpModule = methodInfo->scope; + } +#endif } if (!doInterpret) diff --git a/src/coreclr/interpreter/interpconfigvalues.h b/src/coreclr/interpreter/interpconfigvalues.h index 53ff3cbf7c491a..6376235ba8aae5 100644 --- a/src/coreclr/interpreter/interpconfigvalues.h +++ b/src/coreclr/interpreter/interpconfigvalues.h @@ -23,6 +23,11 @@ RELEASE_CONFIG_METHODSET(Interpreter, "Interpreter") CONFIG_METHODSET(InterpHalt, "InterpHalt"); CONFIG_METHODSET(InterpDump, "InterpDump"); CONFIG_INTEGER(InterpList, "InterpList", 0); // List the methods which are compiled by the interpreter JIT +CONFIG_INTEGER(InterpMode, "InterpMode", 0); // Interpreter mode, one of the following: +// 0: default, do not use interpreter except explicit opt-in via DOTNET_Interpreter +// 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. This mode should have good balance between speed and coverage. +// 2: use interpreter for everything except intrinsics. All intrinsics fallback to JIT. Implies DOTNET_ReadyToRun=0. +// 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0, #undef CONFIG_STRING #undef RELEASE_CONFIG_STRING diff --git a/src/coreclr/interpreter/intops.def b/src/coreclr/interpreter/intops.def index 3bb90e14cb5d1e..14bd5d3ba0f587 100644 --- a/src/coreclr/interpreter/intops.def +++ b/src/coreclr/interpreter/intops.def @@ -397,6 +397,7 @@ OPDEF(INTOP_LEAVE_FILTER, "leavefilter", 2, 0, 1, InterpOpNoArgs) OPDEF(INTOP_LEAVE_CATCH, "leavecatch", 2, 0, 0, InterpOpBranch) OPDEF(INTOP_LOAD_EXCEPTION, "load.exception", 2, 1, 0, InterpOpNoArgs) +OPDEF(INTOP_THROW_PNSE, "throw.pnse", 1, 0, 0, InterpOpNoArgs) OPDEF(INTOP_FAILFAST, "failfast", 1, 0, 0, InterpOpNoArgs) OPDEF(INTOP_GC_COLLECT, "gc.collect", 1, 0, 0, InterpOpNoArgs) diff --git a/src/coreclr/interpreter/intrinsics.cpp b/src/coreclr/interpreter/intrinsics.cpp new file mode 100644 index 00000000000000..df827e0606ffb3 --- /dev/null +++ b/src/coreclr/interpreter/intrinsics.cpp @@ -0,0 +1,128 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#include "interpreter.h" +#include "intrinsics.h" + +#define HAS_PREFIX(haystack, needle) \ + (strncmp(haystack, needle, strlen(needle)) == 0) + +NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE compMethod, CORINFO_METHOD_HANDLE method) +{ + const char* className = NULL; + const char* namespaceName = NULL; + const char* methodName = compHnd->getMethodNameFromMetadata(method, &className, &namespaceName, NULL, 0); + + // Array methods don't have metadata + if (!namespaceName) + return NI_Illegal; + + if (!HAS_PREFIX(namespaceName, "System")) + return NI_Illegal; + + if (!strcmp(namespaceName, "System")) + { + if (!strcmp(className, "Double") || !strcmp(className, "Single")) + { + if (!strcmp(methodName, "ConvertToIntegerNative")) + return NI_PRIMITIVE_ConvertToIntegerNative; + else if (!strcmp(methodName, "MultiplyAddEstimate")) + return NI_System_Math_MultiplyAddEstimate; + } + else if (!strcmp(className, "Debugger")) + { + // Interpreter-FIXME: No NI_ enum value for this? + if (!strcmp(methodName, "Break")) + return NI_Illegal; + } + else if (!strcmp(className, "Math") || !strcmp(className, "MathF")) + { + if (!strcmp(methodName, "ReciprocalEstimate")) + return NI_System_Math_ReciprocalEstimate; + else if (!strcmp(methodName, "ReciprocalSqrtEstimate")) + return NI_System_Math_ReciprocalSqrtEstimate; + } + } + else if (!strcmp(namespaceName, "System.Numerics")) + { + if (!strcmp(className, "Vector") && !strcmp(methodName, "get_IsHardwareAccelerated")) + return NI_IsSupported_False; + + // Fall back to managed implementation for everything else. + return NI_Illegal; + } + else if (!strcmp(namespaceName, "System.Runtime.Intrinsics")) + { + // Vector128 etc + if (HAS_PREFIX(className, "Vector") && !strcmp(methodName, "get_IsHardwareAccelerated")) + return NI_IsSupported_False; + + // Fall back to managed implementation for everything else. + return NI_Illegal; + } + else if (HAS_PREFIX(namespaceName, "System.Runtime.Intrinsics")) + { + // Architecture-specific intrinsics. + if (!strcmp(methodName, "get_IsSupported")) + return NI_IsSupported_False; + + // Every intrinsic except IsSupported is PNSE in interpreted-only mode. + return NI_Throw_PlatformNotSupportedException; + } + else if (HAS_PREFIX(namespaceName, "System.Runtime")) + { + if (!strcmp(namespaceName, "System.Runtime.CompilerServices")) + { + if (!strcmp(className, "Unsafe")) + { + // The members of the S.R.CS.Unsafe namespace have IL generated for them elsewhere in the runtime; + // we want to use that generated IL. It should Just Work once we support unsafe accessors. + // If the JIT is available our fallback to calling the JITted code will also work. + // Interpreter-FIXME: Identify the specific unsafe methods. + return NI_SRCS_UNSAFE_START; + } + else if (!strcmp(className, "StaticsHelpers")) + { + if (!strcmp(methodName, "VolatileReadAsByref")) + return NI_System_Runtime_CompilerServices_StaticsHelpers_VolatileReadAsByref; + } + else if (!strcmp(className, "RuntimeHelpers")) + { + if (!strcmp(methodName, "IsReferenceOrContainsReferences")) + return NI_System_Runtime_CompilerServices_RuntimeHelpers_IsReferenceOrContainsReferences; + } + } + else if (!strcmp(namespaceName, "System.Runtime.InteropServices")) + { + if (!strcmp(className, "MemoryMarshal")) + { + if (!strcmp(methodName, "GetArrayDataReference`1")) + return NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference; + } + } + } + else if (!strcmp(namespaceName, "System.Threading")) + { + if (!strcmp(className, "Interlocked")) + { + if (!strcmp(methodName, "CompareExchange")) + return NI_System_Threading_Interlocked_CompareExchange; + else if (!strcmp(methodName, "MemoryBarrier")) + return NI_System_Threading_Interlocked_MemoryBarrier; + } + else if (!strcmp(className, "Thread")) + { + if (!strcmp(methodName, "FastPollGC")) + return NI_System_Threading_Thread_FastPollGC; + } + else if (!strcmp(className, "Volatile")) + { + if (!strcmp(methodName, "ReadBarrier")) + return NI_System_Threading_Volatile_ReadBarrier; + else if (!strcmp(methodName, "WriteBarrier")) + return NI_System_Threading_Volatile_WriteBarrier; + } + } + + return NI_Illegal; +} diff --git a/src/coreclr/interpreter/intrinsics.h b/src/coreclr/interpreter/intrinsics.h new file mode 100644 index 00000000000000..d48d7b2bc0551a --- /dev/null +++ b/src/coreclr/interpreter/intrinsics.h @@ -0,0 +1,11 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#ifndef __INTERPRETER_INTRINSICS_H__ +#define __INTERPRETER_INTRINSICS_H__ + +#include "../jit/namedintrinsiclist.h" + +NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE compMethod, CORINFO_METHOD_HANDLE method); + +#endif // __INTERPRETER_INTRINSICS_H__ diff --git a/src/coreclr/vm/interpexec.cpp b/src/coreclr/vm/interpexec.cpp index e95739b504fd29..038530d1fccc47 100644 --- a/src/coreclr/vm/interpexec.cpp +++ b/src/coreclr/vm/interpexec.cpp @@ -44,7 +44,7 @@ void InvokeCompiledMethod(MethodDesc *pMD, int8_t *pArgs, int8_t *pRet) } } - pHeader->SetTarget(pMD->GetMultiCallableAddrOfCode()); // The method to call + pHeader->SetTarget(pMD->GetMultiCallableAddrOfCode(CORINFO_ACCESS_ANY)); // The method to call pHeader->Invoke(pHeader->Routines, pArgs, pRet, pHeader->TotalStackSize); } @@ -2391,6 +2391,9 @@ do { \ case INTOP_LEAVE_CATCH: *(const int32_t**)pFrame->pRetVal = ip + ip[1]; goto EXIT_FRAME; + case INTOP_THROW_PNSE: + COMPlusThrow(kPlatformNotSupportedException); + break; case INTOP_FAILFAST: assert(0); break; diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index fe4ef19be4424f..719a4d9f292d76 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -3,6 +3,7 @@ using System; using System.Numerics; +using System.Runtime.Intrinsics; using System.Runtime.CompilerServices; public interface ITest @@ -932,6 +933,10 @@ public static void RunInterpreterTests() if (!TestDelegate()) Environment.FailFast(null); + Console.WriteLine("TestIntrinsics"); + if (!TestIntrinsics()) + Environment.FailFast(null); + Console.WriteLine("TestCalli"); if (!TestCalli()) Environment.FailFast(null); @@ -955,6 +960,33 @@ public static void RunInterpreterTests() Console.WriteLine("All tests passed successfully!"); } + public static bool TestIntrinsics() + { + Console.WriteLine("Vector128.IsHardwareAccelerated="); + Console.WriteLine(Vector128.IsHardwareAccelerated); + Console.WriteLine("X86Base.IsSupported="); + Console.WriteLine(System.Runtime.Intrinsics.X86.X86Base.IsSupported); + Console.WriteLine("ArmBase.IsSupported="); + Console.WriteLine(System.Runtime.Intrinsics.Arm.ArmBase.IsSupported); + + // HACK: The below try block is constructed to always throw and provides an easy way to test how intrinsics behave in the interpreter + // depending on which of our various modes you're in (native fallback, all intrinsics disabled, etc.) + try { + // HACK: FIXME: This console writeline is needed right now because throws at the start of a try block in the interpreter + // aren't currently caught correctly + Console.WriteLine("Inside try block"); + + // NOTE: Depending on the target architecture, these method calls will not have FLG_INTRINSIC, so the interpreter will not + // recognize them as intrinsics and handle them. This *should* be fine, because we conditionally compile CoreLib based on + // architecture, providing pure-managed implementations of the relevant methods that throw a PNSE. + System.Runtime.Intrinsics.X86.X86Base.Pause(); + System.Runtime.Intrinsics.Arm.ArmBase.Yield(); + return false; + } catch (PlatformNotSupportedException) { + return true; + } + } + public static void TestExceptionHandling() { TestTryFinally(); From 475e6179b99db7ee01bdd1ba0e38ea574454f048 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 24 Jun 2025 15:46:22 -0700 Subject: [PATCH 02/10] Debugger.Break is only an intrinsic in mono, not coreclr --- src/coreclr/interpreter/intrinsics.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/interpreter/intrinsics.cpp b/src/coreclr/interpreter/intrinsics.cpp index df827e0606ffb3..5a4552f6dc7af6 100644 --- a/src/coreclr/interpreter/intrinsics.cpp +++ b/src/coreclr/interpreter/intrinsics.cpp @@ -29,12 +29,6 @@ NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE comp else if (!strcmp(methodName, "MultiplyAddEstimate")) return NI_System_Math_MultiplyAddEstimate; } - else if (!strcmp(className, "Debugger")) - { - // Interpreter-FIXME: No NI_ enum value for this? - if (!strcmp(methodName, "Break")) - return NI_Illegal; - } else if (!strcmp(className, "Math") || !strcmp(className, "MathF")) { if (!strcmp(methodName, "ReciprocalEstimate")) From 0c46ef8fb76f9c40dd50d868e542846fb7464e02 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 24 Jun 2025 15:47:04 -0700 Subject: [PATCH 03/10] No special handling needed for Unsafe methods --- src/coreclr/interpreter/intrinsics.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/interpreter/intrinsics.cpp b/src/coreclr/interpreter/intrinsics.cpp index 5a4552f6dc7af6..86accefd92eceb 100644 --- a/src/coreclr/interpreter/intrinsics.cpp +++ b/src/coreclr/interpreter/intrinsics.cpp @@ -72,8 +72,7 @@ NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE comp // The members of the S.R.CS.Unsafe namespace have IL generated for them elsewhere in the runtime; // we want to use that generated IL. It should Just Work once we support unsafe accessors. // If the JIT is available our fallback to calling the JITted code will also work. - // Interpreter-FIXME: Identify the specific unsafe methods. - return NI_SRCS_UNSAFE_START; + return NI_Illegal; } else if (!strcmp(className, "StaticsHelpers")) { From 760b8647f940be701432392f8bdc14b1819d3775 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 8 Jul 2025 13:30:54 -0700 Subject: [PATCH 04/10] InterpMode release build fixes Comment cleanups --- src/coreclr/interpreter/compiler.cpp | 21 +++++++++----------- src/coreclr/interpreter/eeinterp.cpp | 8 +++----- src/coreclr/interpreter/interpconfigvalues.h | 6 +++--- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 445f01180e2283..2f26d6a6018260 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2420,7 +2420,7 @@ void InterpCompiler::EmitPushUnboxAnyNullable(const CORINFO_GENERICHANDLE_RESULT AddIns(INTOP_CALL_HELPER_V_AGS); m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(CORINFO_HELP_UNBOX_NULLABLE); m_pLastNewIns->data[1] = handleData.dataItemIndex; - + m_pLastNewIns->SetSVars2(handleData.genericVar, arg2); m_pLastNewIns->SetDVar(resultVar); } @@ -2520,7 +2520,7 @@ int InterpCompiler::EmitGenericHandleAsVar(const CORINFO_GENERICHANDLE_RESULT &e PushStackType(StackTypeI, NULL); int resultVar = m_pStackPointer[-1].var; m_pStackPointer--; - + GenericHandleData handleData = GenericHandleToGenericHandleData(embedInfo); if (handleData.argType == HelperArgType::GenericResolution) @@ -2622,8 +2622,6 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re m_compHnd->getCallInfo(&resolvedCallToken, pConstrainedToken, m_methodInfo->ftn, flags, &callInfo); if (callInfo.methodFlags & CORINFO_FLG_INTRINSIC) { -// Interpreter-FIXME: Necessary to work around InterpConfig members only being defined in DEBUG configurations. -#if DEBUG if (InterpConfig.InterpMode() >= 3) { NamedIntrinsic ni = GetNamedIntrinsic(m_compHnd, m_methodHnd, callInfo.hMethod); @@ -2633,7 +2631,6 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re return; } } -#endif } if (EmitCallIntrinsics(callInfo.hMethod, callInfo.sig)) @@ -2727,7 +2724,7 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re PushInterpType(ctorType, resolvedCallToken.hClass); CORINFO_GENERICHANDLE_RESULT newObjGenericHandleEmbedInfo; - m_compHnd->embedGenericHandle(&resolvedCallToken, true, m_methodInfo->ftn, &newObjGenericHandleEmbedInfo); + m_compHnd->embedGenericHandle(&resolvedCallToken, true, m_methodInfo->ftn, &newObjGenericHandleEmbedInfo); newObjData = GenericHandleToGenericHandleData(newObjGenericHandleEmbedInfo); } newObjDVar = m_pStackPointer[-2].var; @@ -2875,9 +2872,9 @@ void InterpCompiler::EmitCall(CORINFO_RESOLVED_TOKEN* pConstrainedToken, bool re else if ((callInfo.classFlags & CORINFO_FLG_ARRAY) && newObj) { CORINFO_GENERICHANDLE_RESULT newObjGenericHandleEmbedInfo; - m_compHnd->embedGenericHandle(&resolvedCallToken, true, m_methodInfo->ftn, &newObjGenericHandleEmbedInfo); + m_compHnd->embedGenericHandle(&resolvedCallToken, true, m_methodInfo->ftn, &newObjGenericHandleEmbedInfo); newObjData = GenericHandleToGenericHandleData(newObjGenericHandleEmbedInfo); - + if (newObjData.argType == HelperArgType::GenericResolution) { AddIns(INTOP_NEWMDARR_GENERIC); @@ -5430,17 +5427,17 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) { // Unbox.any of a reference type is just a cast CorInfoHelpFunc castingHelper = m_compHnd->getCastingHelper(&resolvedToken, true /* throwing */); - + CORINFO_GENERICHANDLE_RESULT embedInfo; InterpEmbedGenericResult result; m_compHnd->embedGenericHandle(&resolvedToken, false, m_methodInfo->ftn, &embedInfo); - + EmitPushHelperCall_2(castingHelper, embedInfo, m_pStackPointer[0].var, g_stackTypeFromInterpType[InterpTypeO], NULL); } else { CorInfoHelpFunc helpFunc = m_compHnd->getUnBoxHelper((CORINFO_CLASS_HANDLE)embedInfo.compileTimeHandle); - + if (helpFunc == CORINFO_HELP_UNBOX) { EmitPushUnboxAny(embedInfo, m_pStackPointer[0].var, g_stackTypeFromInterpType[GetInterpType(m_compHnd->asCorInfoType(resolvedToken.hClass))], resolvedToken.hClass); @@ -5483,7 +5480,7 @@ void InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) AddIns(INTOP_NEWARR_GENERIC); m_pLastNewIns->data[0] = GetDataItemIndexForHelperFtn(helpFunc); m_pLastNewIns->data[1] = handleData.dataItemIndex; - + m_pLastNewIns->SetSVars2(handleData.genericVar, newArrLenVar); m_pLastNewIns->SetDVar(m_pStackPointer[-1].var); } diff --git a/src/coreclr/interpreter/eeinterp.cpp b/src/coreclr/interpreter/eeinterp.cpp index 4230cd554e2a73..df0240104a1a35 100644 --- a/src/coreclr/interpreter/eeinterp.cpp +++ b/src/coreclr/interpreter/eeinterp.cpp @@ -55,14 +55,13 @@ CorJitResult CILInterp::compileMethod(ICorJitInfo* compHnd, { // Interpreter-FIXME: Necessary to work around InterpConfig members only being defined in DEBUG. -#if DEBUG switch (InterpConfig.InterpMode()) { // 0: default, do not use interpreter except explicit opt-in via DOTNET_Interpreter case 0: break; - // 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. This mode should have good balance between speed and coverage. + // 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. case 1: { doInterpret = true; @@ -77,16 +76,15 @@ CorJitResult CILInterp::compileMethod(ICorJitInfo* compHnd, doInterpret = !(compHnd->getMethodAttribs(methodInfo->ftn) & CORINFO_FLG_INTRINSIC); break; - // 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0, + // 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0 case 3: doInterpret = true; break; default: - assert(!"Unsupported value for DOTNET_InterpMode"); + NO_WAY("Unsupported value for DOTNET_InterpMode"); break; } -#endif #ifdef TARGET_WASM // interpret everything on wasm diff --git a/src/coreclr/interpreter/interpconfigvalues.h b/src/coreclr/interpreter/interpconfigvalues.h index 6376235ba8aae5..a76685a1fc7011 100644 --- a/src/coreclr/interpreter/interpconfigvalues.h +++ b/src/coreclr/interpreter/interpconfigvalues.h @@ -23,11 +23,11 @@ RELEASE_CONFIG_METHODSET(Interpreter, "Interpreter") CONFIG_METHODSET(InterpHalt, "InterpHalt"); CONFIG_METHODSET(InterpDump, "InterpDump"); CONFIG_INTEGER(InterpList, "InterpList", 0); // List the methods which are compiled by the interpreter JIT -CONFIG_INTEGER(InterpMode, "InterpMode", 0); // Interpreter mode, one of the following: +RELEASE_CONFIG_INTEGER(InterpMode, "InterpMode", 0); // Interpreter mode, one of the following: // 0: default, do not use interpreter except explicit opt-in via DOTNET_Interpreter -// 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. This mode should have good balance between speed and coverage. +// 1: use interpreter for everything except (1) methods that have R2R compiled code and (2) all code in System.Private.CoreLib. All code in System.Private.CoreLib falls back to JIT if there is no R2R available for it. // 2: use interpreter for everything except intrinsics. All intrinsics fallback to JIT. Implies DOTNET_ReadyToRun=0. -// 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0, +// 3: use interpreter for everything, the full interpreter-only mode, no fallbacks to R2R or JIT whatsoever. Implies DOTNET_ReadyToRun=0, DOTNET_EnableHWIntrinsic=0 #undef CONFIG_STRING #undef RELEASE_CONFIG_STRING From 92baf4f0113061fcf49aca66dd3be4e7d5859983 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 8 Jul 2025 13:35:04 -0700 Subject: [PATCH 05/10] Cleanup & address PR feedback --- src/coreclr/interpreter/compiler.cpp | 18 ++++++++++-------- src/coreclr/interpreter/eeinterp.cpp | 2 -- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/interpreter/compiler.cpp b/src/coreclr/interpreter/compiler.cpp index 2f26d6a6018260..4535b1e195e867 100644 --- a/src/coreclr/interpreter/compiler.cpp +++ b/src/coreclr/interpreter/compiler.cpp @@ -2197,20 +2197,22 @@ bool InterpCompiler::EmitNamedIntrinsicCall(NamedIntrinsic ni, CORINFO_CLASS_HAN return true; case NI_Throw_PlatformNotSupportedException: - // Interpreter-FIXME: If an INTOP_THROW_PNSE is the first opcode in a try block, catch doesn't catch the exception - // For now, insert a safepoint as padding. - AddIns(INTOP_SAFEPOINT); AddIns(INTOP_THROW_PNSE); return true; default: { - const char* className = NULL; - const char* namespaceName = NULL; - const char* methodName = m_compHnd->getMethodNameFromMetadata(method, &className, &namespaceName, NULL, 0); - printf("WARNING: Intrinsic not implemented in EmitNamedIntrinsicCall: %d (for %s.%s.%s)\n", ni, namespaceName, className, methodName); +#ifdef DEBUG + if (m_verbose) + { + const char* className = NULL; + const char* namespaceName = NULL; + const char* methodName = m_compHnd->getMethodNameFromMetadata(method, &className, &namespaceName, NULL, 0); + printf("WARNING: Intrinsic not implemented in EmitNamedIntrinsicCall: %d (for %s.%s.%s)\n", ni, namespaceName, className, methodName); + } +#endif if (mustExpand) - assert(!"EmitNamedIntrinsicCall not implemented must-expand intrinsic"); + NO_WAY("EmitNamedIntrinsicCall not implemented must-expand intrinsic"); return false; } } diff --git a/src/coreclr/interpreter/eeinterp.cpp b/src/coreclr/interpreter/eeinterp.cpp index df0240104a1a35..d0351825e06120 100644 --- a/src/coreclr/interpreter/eeinterp.cpp +++ b/src/coreclr/interpreter/eeinterp.cpp @@ -54,7 +54,6 @@ CorJitResult CILInterp::compileMethod(ICorJitInfo* compHnd, doInterpret = true; { -// Interpreter-FIXME: Necessary to work around InterpConfig members only being defined in DEBUG. switch (InterpConfig.InterpMode()) { // 0: default, do not use interpreter except explicit opt-in via DOTNET_Interpreter @@ -95,7 +94,6 @@ CorJitResult CILInterp::compileMethod(ICorJitInfo* compHnd, if (InterpConfig.Interpreter().contains(compHnd, methodInfo->ftn, compHnd->getMethodClass(methodInfo->ftn), &methodInfo->args)) { doInterpret = true; - // FIXME: What if there are multiple modules specified by the DOTNET_Interpreter config variable? g_interpModule = methodInfo->scope; } #endif From b9c8f3893e9f2d0607698c7579f5978cf4bb0f41 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 8 Jul 2025 13:56:55 -0700 Subject: [PATCH 06/10] Remove outdated comment and unnecessary writeline --- src/tests/JIT/interpreter/Interpreter.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 719a4d9f292d76..2817d02aa3a30b 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -972,10 +972,6 @@ public static bool TestIntrinsics() // HACK: The below try block is constructed to always throw and provides an easy way to test how intrinsics behave in the interpreter // depending on which of our various modes you're in (native fallback, all intrinsics disabled, etc.) try { - // HACK: FIXME: This console writeline is needed right now because throws at the start of a try block in the interpreter - // aren't currently caught correctly - Console.WriteLine("Inside try block"); - // NOTE: Depending on the target architecture, these method calls will not have FLG_INTRINSIC, so the interpreter will not // recognize them as intrinsics and handle them. This *should* be fine, because we conditionally compile CoreLib based on // architecture, providing pure-managed implementations of the relevant methods that throw a PNSE. From 1db3566160cbc174f96467b09818c5fc4b747775 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 8 Jul 2025 14:57:13 -0700 Subject: [PATCH 07/10] Update src/coreclr/interpreter/intrinsics.cpp Co-authored-by: Jan Kotas --- src/coreclr/interpreter/intrinsics.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/coreclr/interpreter/intrinsics.cpp b/src/coreclr/interpreter/intrinsics.cpp index 86accefd92eceb..5536675dd9f7ca 100644 --- a/src/coreclr/interpreter/intrinsics.cpp +++ b/src/coreclr/interpreter/intrinsics.cpp @@ -67,14 +67,7 @@ NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE comp { if (!strcmp(namespaceName, "System.Runtime.CompilerServices")) { - if (!strcmp(className, "Unsafe")) - { - // The members of the S.R.CS.Unsafe namespace have IL generated for them elsewhere in the runtime; - // we want to use that generated IL. It should Just Work once we support unsafe accessors. - // If the JIT is available our fallback to calling the JITted code will also work. - return NI_Illegal; - } - else if (!strcmp(className, "StaticsHelpers")) + if (!strcmp(className, "StaticsHelpers")) { if (!strcmp(methodName, "VolatileReadAsByref")) return NI_System_Runtime_CompilerServices_StaticsHelpers_VolatileReadAsByref; From 79a7be1eca0d5d17ff12c51b9a9a3b93dfa79d77 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 9 Jul 2025 05:38:03 -0700 Subject: [PATCH 08/10] Update src/coreclr/interpreter/intrinsics.cpp Co-authored-by: Milos Kotlar --- src/coreclr/interpreter/intrinsics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/interpreter/intrinsics.cpp b/src/coreclr/interpreter/intrinsics.cpp index 5536675dd9f7ca..1e86a3749a7906 100644 --- a/src/coreclr/interpreter/intrinsics.cpp +++ b/src/coreclr/interpreter/intrinsics.cpp @@ -82,7 +82,7 @@ NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE comp { if (!strcmp(className, "MemoryMarshal")) { - if (!strcmp(methodName, "GetArrayDataReference`1")) + if (!strcmp(methodName, "GetArrayDataReference")) return NI_System_Runtime_InteropService_MemoryMarshal_GetArrayDataReference; } } From 4a672ea7812ed353cba542d913299950cf2483ed Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 9 Jul 2025 10:48:09 -0700 Subject: [PATCH 09/10] This test causes crashes on arm64 for some reason and isn't very useful --- src/tests/JIT/interpreter/Interpreter.cs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 2817d02aa3a30b..7635e735360f1a 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -968,19 +968,6 @@ public static bool TestIntrinsics() Console.WriteLine(System.Runtime.Intrinsics.X86.X86Base.IsSupported); Console.WriteLine("ArmBase.IsSupported="); Console.WriteLine(System.Runtime.Intrinsics.Arm.ArmBase.IsSupported); - - // HACK: The below try block is constructed to always throw and provides an easy way to test how intrinsics behave in the interpreter - // depending on which of our various modes you're in (native fallback, all intrinsics disabled, etc.) - try { - // NOTE: Depending on the target architecture, these method calls will not have FLG_INTRINSIC, so the interpreter will not - // recognize them as intrinsics and handle them. This *should* be fine, because we conditionally compile CoreLib based on - // architecture, providing pure-managed implementations of the relevant methods that throw a PNSE. - System.Runtime.Intrinsics.X86.X86Base.Pause(); - System.Runtime.Intrinsics.Arm.ArmBase.Yield(); - return false; - } catch (PlatformNotSupportedException) { - return true; - } } public static void TestExceptionHandling() From 8b30e4b537579537b85a3ea39efcec998867caec Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 9 Jul 2025 11:06:52 -0700 Subject: [PATCH 10/10] Fix build --- src/tests/JIT/interpreter/Interpreter.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/JIT/interpreter/Interpreter.cs b/src/tests/JIT/interpreter/Interpreter.cs index 7635e735360f1a..71a419cfdff908 100644 --- a/src/tests/JIT/interpreter/Interpreter.cs +++ b/src/tests/JIT/interpreter/Interpreter.cs @@ -968,6 +968,7 @@ public static bool TestIntrinsics() Console.WriteLine(System.Runtime.Intrinsics.X86.X86Base.IsSupported); Console.WriteLine("ArmBase.IsSupported="); Console.WriteLine(System.Runtime.Intrinsics.Arm.ArmBase.IsSupported); + return true; } public static void TestExceptionHandling()