Skip to content

Commit

Permalink
JIT: enable devirtualization/inlining of other array interface methods
Browse files Browse the repository at this point in the history 
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<T>.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<T>` 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 dotnet#108913.
  • Loading branch information
@AndyAyersMS
AndyAyersMS committed Oct 25, 2024
1 parent 489a151 commit e4c6f99
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 22 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/inc/corinfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
//
Expand All @@ -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;
};

Expand Down
47 changes: 41 additions & 6 deletions src/coreclr/jit/importercalls.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>
// 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
Expand Down Expand Up @@ -8219,14 +8246,18 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
{
note = "final method";
}
if (dvInfo.requiresInstMethodDescArg)
{
instArg = " + requires inst arg";
}

if (verbose || doPrint)
{
derivedMethodName = eeGetMethodName(derivedMethod);
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);
}
}
Expand Down Expand Up @@ -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++;

Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
//
Expand All @@ -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; } }
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/superpmi/superpmi-shared/agnostic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ struct Agnostic_ResolveVirtualMethodResult
{
bool returnValue;
DWORDLONG devirtualizedMethod;
bool requiresInstMethodTableArg;
bool requiresInstMethodDescArg;
bool wasArrayInterfaceDevirt;
DWORDLONG exactContext;
DWORD detail;
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down
17 changes: 12 additions & 5 deletions src/coreclr/vm/jitinterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit e4c6f99

Please sign in to comment.