JIT: Devirtualize shared generic virtual methods#123323
JIT: Devirtualize shared generic virtual methods#123323hez2010 wants to merge 51 commits intodotnet:mainfrom
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
Pull request overview
This PR enables JIT devirtualization for shared generic virtual methods (GVM) that don't require runtime lookups. Previously, all shared GVMs were blocked from devirtualization due to concerns about having the right generic context. This change unblocks devirtualization when the instantiating stub doesn't need a runtime lookup, by checking for the presence of a GT_RUNTIMELOOKUP node before proceeding.
Changes:
- Introduced
needsMethodContextflag to track when a method context is needed for devirtualization - For shared generic methods, obtain the instantiating stub and set
needsMethodContext = true - Unified handling of array interface and generic virtual method devirtualization paths
- Added runtime lookup check in the JIT to bail out when a lookup is needed but context is unavailable
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/jitinterface.cpp | Added logic to detect shared generic methods and obtain instantiating stubs, unified array and GVM devirtualization handling |
| src/coreclr/jit/importercalls.cpp | Updated assertions to allow GVM in AOT scenarios, added runtime lookup check to prevent devirtualization when context is unavailable |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Failures seem to be caused by missing context during spmi replay. Otherwise all tests are passing. |
hez2010
changed the title
hez2010
force-pushed
the
gvm-devirt-shared
branch
from
bf5b09a to
c8b7f69
Compare
Done in ee80835 (merge) and d87e421(the adaption). It's a great simplification as we no longer need the roundtrip and the weird additional flag which was used to identify whether the arg is coming from |
hez2010
force-pushed
the
gvm-devirt-shared
branch
from
2bcf37a to
d87e421
Compare
src/coreclr/jit/importercalls.cpp
Outdated
| GenTree* instParam = | ||
| getLookupTree(lookupToken, &dvInfo.instParamLookup, GTF_ICON_METHOD_HDL, compileTimeHandle); |
There was a problem hiding this comment.
lookupToken is unused by getLookupTree. I have missed removing it in my change. Can you remove it here and clean up the uses?
| helperArg = new MethodWithToken(methodDesc, HandleToModuleToken(ref pResolvedToken), constrainedType, unboxing: false, context: sharedMethod); | ||
| Debug.Assert(templateMethod != null); | ||
| _compilation.NodeFactory.DetectGenericCycles(MethodBeingCompiled, templateMethod); | ||
| helperArg = ComputeMethodWithToken(templateMethod, ref pResolvedToken, constrainedType: null, unboxing: false); |
There was a problem hiding this comment.
@jakobbotsch This is still not sufficient to get things right.
In non-R2R we emit an ELEMENT_TYPE_INTERNAL to use as the owner type, but I don't see how can I do such things in R2R.
Below is a repro that can reproduce the problem (needs to build with composite R2R with JitAggressiveInlining=1):
Details
public class GenericVirtualMethodTests
{
public static void Main()
{
IBaseMethodCaller caller = new ClassBaseCaller(new ClassBase_GenericDerived_NoInlining<string>());
string value = "repro";
Equal(value, RuntimeLookupBridgeShared<string>.SameClassDifferentMethod(caller, value));
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static void Equal<T>(T expected, T actual, [CallerArgumentExpression(nameof(actual))] string testcase = "")
{
Console.WriteLine("Validating {0}...", testcase);
if (!Equals(expected, actual))
{
throw new Exception($"Validation failed: expected '{expected}', got '{actual}'.");
}
}
}
internal static class RuntimeLookupBridgeShared<TMethod>
{
public static TMethod SameClassDifferentMethod(IBaseMethodCaller caller, TMethod value)
{
return RuntimeLookupDispatcher<string>.SameClassDifferentMethod(caller, value);
}
}
internal static class RuntimeLookupDispatcher<TContext>
{
[MethodImpl(MethodImplOptions.NoInlining)]
public static TMethod SameClassDifferentMethod<TMethod>(IBaseMethodCaller caller, TMethod value)
{
RuntimeLookupVirtualInvoker invoker = new RuntimeLookupVirtualStage<TContext>();
return invoker.SameClassDifferentMethod(caller, value);
}
}
internal abstract class RuntimeLookupVirtualInvoker
{
public abstract T SameClassDifferentMethod<T>(IBaseMethodCaller caller, T value);
}
internal sealed class RuntimeLookupVirtualStage<TContext> : RuntimeLookupVirtualInvoker
{
[MethodImpl(MethodImplOptions.NoInlining)]
public override T SameClassDifferentMethod<T>(IBaseMethodCaller caller, T value)
{
return SameClassDifferentMethodCore(caller, value);
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static T SameClassDifferentMethodCore<T>(IBaseMethodCaller caller, T value)
{
RuntimeLookupVirtualInvoker invoker = RuntimeLookupTerminalFactory.CreateInvoker();
return invoker.SameClassDifferentMethod(caller, value);
}
}
internal static class RuntimeLookupTerminalFactory
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static RuntimeLookupVirtualInvoker CreateInvoker()
{
return new RuntimeLookupTerminalInvoker();
}
}
internal sealed class RuntimeLookupTerminalInvoker : RuntimeLookupVirtualInvoker
{
[MethodImpl(MethodImplOptions.NoInlining)]
public override T SameClassDifferentMethod<T>(IBaseMethodCaller caller, T value)
{
return SameClassDifferentMethodCore(caller, value);
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static T SameClassDifferentMethodCore<T>(IBaseMethodCaller caller, T value)
{
return caller.Invoke(value);
}
}
internal interface IBaseMethodCaller
{
T Invoke<T>(T value);
}
internal sealed class ClassBaseCaller : IBaseMethodCaller
{
private readonly NonGenericBaseClass _instance;
public ClassBaseCaller(NonGenericBaseClass instance)
{
_instance = instance;
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public T Invoke<T>(T value) => _instance.Process(value);
}
internal abstract class NonGenericBaseClass
{
public abstract T Process<T>(T value);
}
internal sealed class ClassBase_GenericDerived_NoInlining<TDerived> : NonGenericBaseClass
{
[MethodImpl(MethodImplOptions.NoInlining)]
public override T Process<T>(T value) => value;
}It will perform a runtime lookup, then it tries to obtain the owner type at
but we don't actually have one in the signature, so it tries to load the wrong thing (in this case it's trying to load a ELEMENT_TYPE_VAR).
I have disabled the runtime lookup support in R2R in 154f832. Would like to hear what you think about this.
|
The output of this program is wrong on coreclr with this PR: using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
public class Program
{
public static void Main()
{
Test<string>();
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static void Test<T>()
{
Derived<T> foo = new();
foo.Test<object>();
}
public class Base<T>
{
public virtual void Test<U>()
{
Console.WriteLine(typeof(T).FullName);
Console.WriteLine(typeof(U).FullName);
}
}
public class Derived<T> : Base<List<T>>
{
}
}I am not sure that using the token that we get from the base will work. Under any circumstances it is not the right thing to instantiate the base class type parameters with. Some changes to the signature will be needed to apply the proper modifications coming from the inheritance hierarchy. |
|
The VM uses a concept called Substitution to make that sort of thing work, but that's at the metadata level, and not typically directly used in this portion of the runtime. For overrides is typically done by starting from an actual |
|
Notably, to make this all work, you may need to encode 2 different types in the as part of |
|
I'm also not entirely sure which scenarios you are trying to optimize here. The nicest examples I see above are entirely removing the virtual dispatch, but I can imagine scenarios where you could be using devirtualization to remove just the virtual lookup operation. This isn't commonly all that useful on regular virtual dispatch, or even in interface dispatch as the dispatch mechanisms are actually quite fast, but in GVM dispatch there is a hashtable involved, which could be removed to somewhat good effect. |
Yeah. I'm not actually sure how we can make this work. We won't have the exact object type available during either The only thing I can think of is a special version of the runtime lookup helper that extracts the exact type from the object and then finds the overriding method table that way. Maybe I am missing something. |
|
I could see something like... there is an explicit call to newobj in the method being compiler followed by a use of a GVM on that object. In that sort of situation, you could use the token associated with the newobj to define the exact type and then work from there. Similarly, if there is a type which is sealed in a signature, you could do similar tricks, even if you couldn't see the exact type. This is why I'm trying to figure out which scenarios are expected to be optimizable with this change. |
|
The hope would be that most GVMs where we know the (possibly shared) exact type would be optimizable by devirtualization. We should be able to compute the exact target and hence avoid the virtual resolution and even do inlining of it, but it seems like computing the instantiation argument is very difficult when the JIT does not know the unshared exact type. (In many cases we know exact unshared types, e.g. under guarded devirtualization that would be the case.) Perhaps this PR should should focus on the exact unshared types to start out with. Obviously computing the instantiation argument for those is much simpler since no runtime lookup is required. |
|
Thanks for the explanation. I'm going to remove the part that tries to compute the runtime lookup and instead only focus on cases where we know the exact type in this PR. |
The scenario I was thinking about is not the lookup itself. It's that by turning the GVM call into a directly call, it enables the JIT to inline the callee despite it still needs a runtime lookup. |
|
I think it is impossible in general to represent the instantiation argument as a runtime lookup because the JIT does not guarantee that exact shared types have unique instantiations. Consider using System;
using System.Runtime.CompilerServices;
public class Program
{
public static void Main()
{
CallTest<string, object>(true);
}
[MethodImpl(MethodImplOptions.NoInlining)]
private static void CallTest<T, U>(bool b)
{
Base foo = GetBase<T, U>(b);
foo.Test<object>();
}
private static Base GetBase<T, U>(bool b)
{
if (b)
{
return new Derived<T>();
}
else
{
return new Derived<U>();
}
}
public class Base
{
public virtual void Test<U>()
{
Console.WriteLine(typeof(U).FullName);
}
}
public class Derived<T> : Base
{
public override void Test<U>()
{
Console.WriteLine(typeof(T).FullName);
Console.WriteLine(typeof(U).FullName);
}
}
}After inlining However, it is not possible to compute the instantiation argument to pass to A few alternatives:
(3) might still be beneficial because we'll be able to inline the target code and in many cases the instantiation argument completely disappears after that. But I would probably start with (1) and perhaps (2). |
4 participants
Previously in #122023, we hit an issue with GVM devirtualization when the devirtualized target is a shared generic method. GVM calls are imported with a runtime lookup that is specific to the base method. After devirtualization, the call requires the instantiation argument for the implementing method, and the existing lookup cannot be reused.
This PR unblocks devirtualization for shared generic targets by ensuring the call receives the correct instantiation parameter for the devirtualized method:
The multiple ad-hoc flags in
dvInfonow have been unified into a singleinstParamLookupWhen the target does not require a runtime lookup, we already know the exact generic context. We pass the instantiating stub as the inst param (shared with the existing array interface devirtualization path).
Store the instantiating stub (when necessary) directly in the exactContext, and devirtualizedMethod now can never be an instantiating stub. Remove the unnecessary
getInstantiatedEntryroundtrip.When the target requires a runtime lookup, we now introduced a new
DictionaryEntryKind::DevirtualizedMethodDescSlot, and pass it to theinstParamLookupso that later the VM knows that it needs to encode the class token from the devirtualized method instead of the original token. And in this case, the devirtualized methodpDevirtMDwill be passed as template method.Also due to the
instParamLookupchange I implement the support for R2R as well.NativeAOT still needs extra work in JIT to enable GVM devirts.
Example:
Codegen diff:
Another example that involves runtime lookup:
Codegen diff:
Contributes to #112596