Return type from exact method context for call returns

[MichalPetryka]

This should let us devirtualize and inline more calls due to seeing the real type in variables instead of _Canon.

I've reused the existing LateDevirtualizationInfo for this and enabled storing it for non virtual calls too, it seemed safe as far as I've seen as all other places still checked IsVirtual before using it.

[MichalPetryka]

@MihuBot

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[AndyAyersMS]

@MichalPetryka what's the status of this PR?

[MichalPetryka]

@MichalPetryka what's the status of this PR?

NativeAOT/R2R VM doesn't like seeing non shared generic contexts here and iirc my last discussion about it with @MichalStrehovsky ended up with me asking whether that VM should be changed to behave like coreclr or if the JIT should check stuff here.

[jkotas]

NativeAOT/R2R VM doesn't like seeing non shared generic contexts here

Where exactly?

[MichalPetryka]

Where exactly?

In the getMethodSig path with the 2 asserts I've changed in this PR to have better messages.

[jkotas]

The JIT/EE interface methods expect that the context and method should always match. If you are passing mismatched context and method around, it won't work well. I would expect that to hit problems in both AOT and JIT scenarios.

Maybe you need a new method on JIT/EE interface to compute context for a method from more derived type?

[MichalStrehovsky]

NativeAOT/R2R VM doesn't like seeing non shared generic contexts here

Where exactly?

I think something along the lines of this will fix the problem:

diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
index 2be89e7374f..07bdb48b21a 100644
--- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
+++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
@@ -1452,7 +1452,14 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO
                     // the method with a method the intrinsic expands into. If it's not the special intrinsic,
                     // method stays unchanged.
                     var methodIL = (MethodIL)HandleToObject((void*)pResolvedToken.tokenScope);
-                    targetMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod);
+                    MethodDesc callsiteMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod);
+                    if (targetMethod != callsiteMethod)
+                    {
+                        Debug.Assert(!pResult->exactContextNeedsRuntimeLookup);
+                        Debug.Assert(!callsiteMethod.HasInstantiation && !targetMethod.HasInstantiation);
+                        pResult->contextHandle = contextFromType(callsiteMethod.OwningType);
+                        targetMethod = callsiteMethod;
+                    }

                     // For multidim array Address method, we pretend the method requires a hidden instantiation argument
                     // (even though it doesn't need one). We'll actually swap the method out for a differnt one with

I'd undo the assert changes.

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[tannergooding]

@MichalPetryka asked for this to be reopened.

[MichalPetryka]

@MihuBot

[MichalPetryka]

NativeAOT/R2R VM doesn't like seeing non shared generic contexts here

Where exactly?

I think something along the lines of this will fix the problem:

diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
index 2be89e7374f..07bdb48b21a 100644
--- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
+++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
@@ -1452,7 +1452,14 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO
                     // the method with a method the intrinsic expands into. If it's not the special intrinsic,
                     // method stays unchanged.
                     var methodIL = (MethodIL)HandleToObject((void*)pResolvedToken.tokenScope);
-                    targetMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod);
+                    MethodDesc callsiteMethod = _compilation.ExpandIntrinsicForCallsite(targetMethod, methodIL.OwningMethod);
+                    if (targetMethod != callsiteMethod)
+                    {
+                        Debug.Assert(!pResult->exactContextNeedsRuntimeLookup);
+                        Debug.Assert(!callsiteMethod.HasInstantiation && !targetMethod.HasInstantiation);
+                        pResult->contextHandle = contextFromType(callsiteMethod.OwningType);
+                        targetMethod = callsiteMethod;
+                    }

                     // For multidim array Address method, we pretend the method requires a hidden instantiation argument
                     // (even though it doesn't need one). We'll actually swap the method out for a differnt one with

I'd undo the assert changes.

Seems fine with this, can't say I fully understand how it fixes the issue though.

[MichalPetryka]

@MihuBot merge

[AndyAyersMS]

The diffs from this are very minimal, which is a little surprising. As is I am not sure we should take this change. Either this particular pattern doesn't come up very often, or there is something else inhibiting the JIT from acting on this information.

You should consider trying to instrument to see how often this produces a "better" type for the return value, and then look into cases where we have a better return type but no visible impact, and see if there are any blockers.

[JulieLeeMSFT]

Ping @MichalPetryka on where you are on this PR and if you will address the feedback from andyayersMS.

[dotnet-policy-service]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[dotnet-policy-service]

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.