From e4c6f996d4c34a0bd79edb2ae617d12e9cc92a4f Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 24 Oct 2024 17:19:38 -0700 Subject: [PATCH] JIT: enable devirtualization/inlining of other array interface methods The JIT recently enabled devirtualization of `GetEnumerator`, but other methods were inhibited from devirtualization because the runtime was returning an instantiating stub instead of the actual method. This blocked inlining and the JIT currently will not GDV unless it can also inline. So for instance `ICollection.Count` would not devirtualize. We think we know enough to pass the right inst parameter (the exact method desc) so enable this for the array case, at least for normal jitting. For NAOT array devirtualization happens via normal paths thanks to `Array` so should already fpr these cases. For R2R we don't do array interface devirt (yet). There was an existing field on `CORINFO_DEVIRTUALIZATION_INFO` to record the need for an inst parameter, but it was unused and so I renamed it and use it for this case. Contributes to #108913. --- src/coreclr/inc/corinfo.h | 4 +- src/coreclr/jit/importercalls.cpp | 47 ++++++++++++++++--- .../tools/Common/JitInterface/CorInfoImpl.cs | 4 +- .../tools/Common/JitInterface/CorInfoTypes.cs | 6 +-- .../tools/superpmi/superpmi-shared/agnostic.h | 2 +- .../superpmi-shared/methodcontext.cpp | 6 +-- src/coreclr/vm/jitinterface.cpp | 17 +++++-- 7 files changed, 64 insertions(+), 22 deletions(-) diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 4f99061c5e1a18..4d45aeb3b1c75f 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -1511,7 +1511,7 @@ struct CORINFO_DEVIRTUALIZATION_INFO // - details on the computation done by the jit host // - If pResolvedTokenDevirtualizedMethod is not set to NULL and targeting an R2R image // use it as the parameter to getCallInfo - // - requiresInstMethodTableArg is set to TRUE if the devirtualized method requires a type handle arg. + // - requiresInstMethodDescArg is set to TRUE if the devirtualized method requires a method desc arg. // - wasArrayInterfaceDevirt is set TRUE for array interface method devirtualization // (in which case the method handle and context will be a generic method) // @@ -1520,7 +1520,7 @@ struct CORINFO_DEVIRTUALIZATION_INFO CORINFO_DEVIRTUALIZATION_DETAIL detail; CORINFO_RESOLVED_TOKEN resolvedTokenDevirtualizedMethod; CORINFO_RESOLVED_TOKEN resolvedTokenDevirtualizedUnboxedMethod; - bool requiresInstMethodTableArg; + bool requiresInstMethodDescArg; bool wasArrayInterfaceDevirt; }; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 24f2ddb17a4cec..4a75f6118463d3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -8189,8 +8189,35 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, const char* derivedClassName = "?derivedClass"; const char* derivedMethodName = "?derivedMethod"; const char* note = "inexact or not final"; + const char* instArg = ""; #endif + if (dvInfo.requiresInstMethodDescArg) + { + // We should only end up with generic methods for array interface devirt. + // + assert(dvInfo.wasArrayInterfaceDevirt); + + // We don't expect NAOT to end up here, since it has Array + // and normal devirtualization. + // + assert(!IsTargetAbi(CORINFO_NATIVEAOT_ABI)); + + // We don't expect R2R to end up here, since it does not (yet) support + // array interface devirtualization. + // + assert(!opts.IsReadyToRun()); + + // We don't expect there to be an existing inst param arg. + // + CallArg* const instParam = call->gtArgs.FindWellKnownArg(WellKnownArg::InstParam); + if (instParam != nullptr) + { + assert(!"unexpected inst param in virtual/interface call"); + return; + } + } + // If we failed to get a method handle, we can't directly devirtualize. // // This can happen when prejitting, if the devirtualization crosses @@ -8219,6 +8246,10 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, { note = "final method"; } + if (dvInfo.requiresInstMethodDescArg) + { + instArg = " + requires inst arg"; + } if (verbose || doPrint) { @@ -8226,7 +8257,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, derivedClassName = eeGetClassName(derivedClass); if (verbose) { - printf(" devirt to %s::%s -- %s\n", derivedClassName, derivedMethodName, note); + printf(" devirt to %s::%s -- %s%s\n", derivedClassName, derivedMethodName, note, instArg); gtDispTree(call); } } @@ -8267,11 +8298,6 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // All checks done. Time to transform the call. // - // We should always have an exact class context. - // - // Note that wouldnt' be true if the runtime side supported array interface devirt, - // the resulting method would be a generic method of the non-generic SZArrayHelper class. - // assert(canDevirtualize); Metrics.DevirtualizedCall++; @@ -8285,6 +8311,15 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, call->gtControlExpr = nullptr; INDEBUG(call->gtCallDebugFlags |= GTF_CALL_MD_DEVIRTUALIZED); + if (dvInfo.requiresInstMethodDescArg) + { + // Pass the method desc as the inst param arg. + // Need to make sure this works in all cases. We might need more complex embedding. + // + GenTree* const instParam = gtNewIconNode((ssize_t)derivedMethod, TYP_I_IMPL); + call->gtArgs.InsertInstParam(this, instParam); + } + // Virtual calls include an implicit null check, which we may // now need to make explicit. if (!objIsNonNull) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 5038448f8cafc4..18fb2c06f91aa1 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1302,7 +1302,7 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) info->devirtualizedMethod = null; info->exactContext = null; info->detail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_UNKNOWN; - info->requiresInstMethodTableArg = false; + info->requiresInstMethodDescArg = false; info->wasArrayInterfaceDevirt = false; TypeDesc objType = HandleToObject(info->objClass); @@ -1450,7 +1450,7 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) #endif info->detail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_SUCCESS; info->devirtualizedMethod = ObjectToHandle(impl); - info->requiresInstMethodTableArg = false; + info->requiresInstMethodDescArg = false; info->exactContext = contextFromType(owningType); return true; diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs index b6fbb9cbb84ae4..9be2cf8ffd2429 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs @@ -1083,7 +1083,7 @@ public unsafe struct CORINFO_DEVIRTUALIZATION_INFO // invariant is `resolveVirtualMethod(...) == (devirtualizedMethod != nullptr)`. // - exactContext is set to wrapped CORINFO_CLASS_HANDLE of devirt'ed method table. // - detail describes the computation done by the jit host - // - requiresInstMethodTableArg is set to TRUE if the devirtualized method requires a type handle arg. + // - requiresInstMethodDescArg is set to TRUE if the devirtualized method requires a method desc arg. // - wasArrayInterfaceDevirt is set TRUE for array interface method devirtualization // (in which case the method handle and context will be a generic method) // @@ -1092,8 +1092,8 @@ public unsafe struct CORINFO_DEVIRTUALIZATION_INFO public CORINFO_DEVIRTUALIZATION_DETAIL detail; public CORINFO_RESOLVED_TOKEN resolvedTokenDevirtualizedMethod; public CORINFO_RESOLVED_TOKEN resolvedTokenDevirtualizedUnboxedMethod; - public byte _requiresInstMethodTableArg; - public bool requiresInstMethodTableArg { get { return _requiresInstMethodTableArg != 0; } set { _requiresInstMethodTableArg = value ? (byte)1 : (byte)0; } } + public byte _requiresInstMethodDescArg; + public bool requiresInstMethodDescArg { get { return _requiresInstMethodDescArg != 0; } set { _requiresInstMethodDescArg = value ? (byte)1 : (byte)0; } } public byte _wasArrayInterfaceDevirt; public bool wasArrayInterfaceDevirt { get { return _wasArrayInterfaceDevirt != 0; } set { _wasArrayInterfaceDevirt = value ? (byte)1 : (byte)0; } } } diff --git a/src/coreclr/tools/superpmi/superpmi-shared/agnostic.h b/src/coreclr/tools/superpmi/superpmi-shared/agnostic.h index a0138db26532b5..946e29e452d443 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/agnostic.h +++ b/src/coreclr/tools/superpmi/superpmi-shared/agnostic.h @@ -654,7 +654,7 @@ struct Agnostic_ResolveVirtualMethodResult { bool returnValue; DWORDLONG devirtualizedMethod; - bool requiresInstMethodTableArg; + bool requiresInstMethodDescArg; bool wasArrayInterfaceDevirt; DWORDLONG exactContext; DWORD detail; diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 8455abdb80d61c..8a2c6b208a0fc1 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -3290,7 +3290,7 @@ void MethodContext::recResolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO * info Agnostic_ResolveVirtualMethodResult result; result.returnValue = returnValue; result.devirtualizedMethod = CastHandle(info->devirtualizedMethod); - result.requiresInstMethodTableArg = info->requiresInstMethodTableArg; + result.requiresInstMethodDescArg = info->requiresInstMethodDescArg; result.exactContext = CastHandle(info->exactContext); result.detail = (DWORD) info->detail; result.wasArrayInterfaceDevirt = info->wasArrayInterfaceDevirt; @@ -3321,7 +3321,7 @@ void MethodContext::dmpResolveVirtualMethod(const Agnostic_ResolveVirtualMethodK printf(", value returnValue-%s, devirtMethod-%016" PRIX64 ", requiresInstArg-%s, wasArrayInterfaceDevirt-%s, exactContext-%016" PRIX64 ", detail-%d, tokDvMeth{%s}, tokDvUnboxMeth{%s}", result.returnValue ? "true" : "false", result.devirtualizedMethod, - result.requiresInstMethodTableArg ? "true" : "false", + result.requiresInstMethodDescArg ? "true" : "false", result.wasArrayInterfaceDevirt ? "true" : "false", result.exactContext, result.detail, @@ -3346,7 +3346,7 @@ bool MethodContext::repResolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO * info DEBUG_REP(dmpResolveVirtualMethod(key, result)); info->devirtualizedMethod = (CORINFO_METHOD_HANDLE) result.devirtualizedMethod; - info->requiresInstMethodTableArg = result.requiresInstMethodTableArg; + info->requiresInstMethodDescArg = result.requiresInstMethodDescArg; info->wasArrayInterfaceDevirt = result.wasArrayInterfaceDevirt; info->exactContext = (CORINFO_CONTEXT_HANDLE) result.exactContext; info->detail = (CORINFO_DEVIRTUALIZATION_DETAIL) result.detail; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 8d16aa72051147..fe7ca81a384655 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8561,7 +8561,7 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) info->detail = CORINFO_DEVIRTUALIZATION_UNKNOWN; memset(&info->resolvedTokenDevirtualizedMethod, 0, sizeof(info->resolvedTokenDevirtualizedMethod)); memset(&info->resolvedTokenDevirtualizedUnboxedMethod, 0, sizeof(info->resolvedTokenDevirtualizedUnboxedMethod)); - info->requiresInstMethodTableArg = false; + info->requiresInstMethodDescArg = false; info->wasArrayInterfaceDevirt = false; MethodDesc* pBaseMD = GetMethod(info->virtualMethod); @@ -8765,21 +8765,28 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) // Success! Pass back the results. // - info->devirtualizedMethod = (CORINFO_METHOD_HANDLE) pDevirtMD; - if (isArray) { + // If array devirtualization produced an instantiation stub, + // find the wrapped method and return that instead. + // + if (pDevirtMD->IsInstantiatingStub()) + { + pDevirtMD = pDevirtMD->GetWrappedMethodDesc(); + } + info->exactContext = MAKE_METHODCONTEXT((CORINFO_METHOD_HANDLE) pDevirtMD); - info->requiresInstMethodTableArg = pDevirtMD->RequiresInstMethodTableArg(); + info->requiresInstMethodDescArg = pDevirtMD->RequiresInstMethodDescArg(); info->wasArrayInterfaceDevirt = true; } else { info->exactContext = MAKE_CLASSCONTEXT((CORINFO_CLASS_HANDLE) pExactMT); - info->requiresInstMethodTableArg = false; + info->requiresInstMethodDescArg = false; info->wasArrayInterfaceDevirt = false; } + info->devirtualizedMethod = (CORINFO_METHOD_HANDLE) pDevirtMD; info->detail = CORINFO_DEVIRTUALIZATION_SUCCESS; return true;