-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return type from exact method context for call returns #100041
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@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. |
Where exactly? |
In the |
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? |
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. |
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
@MichalPetryka asked for this to be reopened. |
Seems fine with this, can't say I fully understand how it fixes the issue though. |
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@MihuBot merge |
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. |
Ping @MichalPetryka on where you are on this PR and if you will address the feedback from andyayersMS. |
This pull request has been automatically marked |
This pull request will now be closed since it had been marked |
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 checkedIsVirtual
before using it.