Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Track return type for shared generics via exact context - #129373

Open
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context
Open

Track return type for shared generics via exact context#129373
MichalPetryka wants to merge 6 commits into
dotnet:mainfrom
MichalPetryka:return-context

Conversation

@MichalPetryka

@MichalPetrykaMichalPetryka commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Retrying #100041, seems to work locally but not sure if everything is correct since I was getting a bunch of asserts.

Example code this improves: https://compiler-explorer.com/z/q437fGGnW

@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 13, 2026
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Comment threadsrc/coreclr/jit/fginline.cpp
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@jkotas
jkotas marked this pull request as ready for review June 14, 2026 14:03
CopilotAI review requested due to automatic review settings June 14, 2026 14:03

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends CoreCLR JIT “exact context” tracking so that calls with shared generic return types can later recover a more precise return type (avoiding __Canon), enabling improved downstream optimizations (e.g., devirtualization / inlining decisions based on sharper types).

Changes:

  • Introduces ExactContextInfo (renamed from LateDevirtualizationInfo) and stores it on GT_CALL nodes for virtual calls and certain shared-generic-return calls.
  • Uses the saved exact context in gtGetClassHandle to query the EE for a more precise call return type.
  • Adjusts NativeAOT/CoreCLR tooling JitInterface logic around method signature instantiation and callsite-specific intrinsic expansion context.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
src/coreclr/vm/methodtable.cppAdds a reverse hierarchy lookup when retrieving parent instantiation.
src/coreclr/tools/Common/JitInterface/CorInfoImpl.csChanges how memberParent is handled when computing a method signature.
src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.csUpdates contextHandle when an intrinsic expands to a different callsite-specific helper method.
src/coreclr/jit/inline.hRenames LateDevirtualizationInfoExactContextInfo and updates associated comment.
src/coreclr/jit/importercalls.cppSaves ExactContextInfo for virtual calls and shared generic returns.
src/coreclr/jit/gentree.hRenames the per-call field to gtExactContextInfo.
src/coreclr/jit/gentree.cppInitializes/clones gtExactContextInfo and uses it to refine return-type signature queries.
src/coreclr/jit/fginline.cppUpdates late devirtualization to use gtExactContextInfo.
Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:1233

  • This change avoids the Debug.Assert by skipping re-instantiation when memberParent isn't the same type definition as method.OwningType. However, RyuJIT can pass a derived type as memberParent (e.g., Derived when the method is declared on Base), and in that case we still need to locate the corresponding instantiation of the owning type up the BaseType chain to produce the correct signature. Otherwise the returned signature can remain canonical/shared (e.g., with __Canon) and defeat the purpose of passing memberParent.
 else if (type is InstantiatedType instantiatedType && method.OwningType.HasSameTypeDefinition(instantiatedType))
{
Instantiation methodInst = method.Instantiation;
method = _compilation.TypeSystemContext.GetMethodForInstantiatedType(method.GetTypicalMethodDefinition(), instantiatedType);
if (methodInst.Length > 0)
{
method = method.MakeInstantiatedMethod(methodInst);
}
}

Comment threadsrc/coreclr/jit/gentree.cpp Outdated
Comment threadsrc/coreclr/vm/methodtable.cpp Outdated
Comment threadsrc/coreclr/jit/inline.h
Update the exact context info after late devirtualization
@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

@MihuBot -nuget

Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Comment threadsrc/coreclr/jit/fginline.cpp Outdated
Co-authored-by: Steve <hez2010@outlook.com>
@MihaZupan

Copy link
Copy Markdown
Member

@MihuBot -nuget -jitutils-repo EgorBo/jitutils -jitutils-branch pmi-deterministic-cctors

@MichalPetryka

Copy link
Copy Markdown
ContributorAuthor

The bot shows a bunch of diffs in NuGet libraries but only 2 ones in the BCL so it seems like the BCL just doesn't hit this pattern?

@AndyAyersMS does this answer your question from the last time?

@github-actions

github-actionsBot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_reviewed_commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f36379692fdd0448b5b1276a1a4aa075a0d2c1e0",
"last_recorded_worker_run_id": "29676143510",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e91c12de49c642b60b9be49c91e8421c1c289596",
"review_id": 4730524423
}
]
}

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: When the JIT queries a call's return type via gtGetClassHandle for a user call, it previously discarded the exact generic context, so for shared generic instantiations it could only recover an approximate (canonical) return type. This blocks type-driven optimizations on values flowing out of such calls. The PR revives the earlier attempt (#100041) to preserve the exact context so the precise return type is available. Example improvement: (compilerexplorer.com/redacted)

Approach: The existing per-call LateDevirtualizationInfo side structure is generalized and renamed to ExactContextInfo (gtLateDevirtualizationInfo -> gtExactContextInfo). It is now populated not only for devirtualization candidates but also for calls whose signature return type is a shared instantiation (eeIsSharedInst(sig->retTypeSigClass), excluding byrefs). A new eeGetMethodFromContext helper factors the method-context decoding out of eeGetClassFromContext. gtGetClassHandle consumes the saved context to compute an exactClass (and, for method contexts, an exact method) before calling eeGetMethodSig, yielding the precise return type. The RyuJit AOT getCallInfo path is updated to set contextHandle when intrinsic-for-callsite expansion swaps the target method, so the JIT sees a consistent exact context in that case.

Summary: The change is well-scoped and internally consistent. The rename is applied uniformly (no stale LateDevirtualizationInfo references remain), gtExactContextInfo is initialized in gtNewCallNode and copied in gtCloneExprCallHelper, and the unconditional deref in LateDevirtualization remains safe because the field is still always set for devirtualization candidates. All new reads outside that path are null-guarded, and eeGetMethodFromContext asserts its context encoding. The main residual risk is correctness of the newly-derived exact return types across the full generics matrix (CoreCLR and NativeAOT) — the author noted encountering asserts while iterating — so this is primarily validated by broad JIT/pri1 and NativeAOT CI coverage rather than by review of the diff alone. No blocking issues found in the changed lines. Verdict: LGTM pending green CI.

Detailed Findings

No actionable defects identified in the changed lines. Observations:

  • gtGetClassHandle (src/coreclr/jit/gentree.cpp): for a CLASS-flagged context, method remains the (canonical) gtCallMethHnd while exactClass is set from the context; for a METHOD-flagged context both method and exactClass are refined. This asymmetry is intentional and correct, but relies on eeGetMethodSig producing the exact return type from the approximate method + exact class in the CLASS case; worth confirming via CI on shared value-type instantiations.
  • The sig->retType != CORINFO_TYPE_BYREF exclusion in importercalls.cpp is a reasonable guard (byref-returning shared generics don't yield a usable return class), but is undocumented; a brief comment on why byref is excluded would aid future readers.
  • The NativeAOT getCallInfo change asserts !exactContextNeedsRuntimeLookup and no instantiations on the intrinsic-expansion swap path; these assumptions look sound for the ExpandIntrinsicForCallsite cases but are the most AOT-specific part of the change and should be exercised by the NativeAOT test legs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 69.6 AIC · ⌖ 10.5 AIC · ⊞ 10K

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@MichalPetryka@MihaZupan@hez2010