Skip to content

Use common helper to decide runtime lookups for code pointers#129560

Draft
MichalStrehovsky wants to merge 1 commit into
mainfrom
MichalStrehovsky-patch-2
Draft

Use common helper to decide runtime lookups for code pointers#129560
MichalStrehovsky wants to merge 1 commit into
mainfrom
MichalStrehovsky-patch-2

Conversation

@MichalStrehovsky

Copy link
Copy Markdown
Member

Instead of always using R2R helper, decide the appropriate lookup strategy with the shared helper.

Instead of always using R2R helper, decide the appropriate lookup strategy with the shared helper.
Copilot AI review requested due to automatic review settings June 18, 2026 04:06
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 18, 2026
@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.

Copilot AI 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 updates NativeAOT’s RyuJIT interface to compute method code-pointer runtime lookups using the shared ComputeLookup helper, rather than always forcing a ReadyToRun helper-based generic lookup. This aligns the MethodEntry lookup path with other callsite lookup paths that already use ComputeLookup.

Changes:

  • Replace manual CORINFO_LOOKUP initialization for fat-function-pointer method entry lookups with a single call to ComputeLookup.
  • Centralize the choice of lookup strategy (constant vs runtime lookup; helper vs fixed indirections/null) under the shared lookup computation logic.

@github-actions

Copy link
Copy Markdown
Contributor

Copilot Code Review

Holistic Assessment

Motivation: The change eliminates duplicated inline runtime-lookup setup code in favor of the existing ComputeLookup shared helper that already handles the same logic (and more) for other code paths in the same method. This is well-motivated — the same pattern is already used at 5+ other call sites within getCallInfo.

Approach: Correct approach. The replaced block at line ~1457 (fat function pointer + exactContextNeedsRuntimeLookup) was the only remaining call site in getCallInfo that manually set up the R2R helper path instead of delegating to ComputeLookup. The identical pattern at line 1437 (constrained direct call with MethodEntry) already used ComputeLookup, making this a natural consistency fix.

Summary: ✅ LGTM. Small, focused refactoring that replaces hardcoded R2R-helper-only logic with the shared ComputeLookup helper, making this code path consistent with all other runtime lookup sites in the method. The behavioral change — allowing the compilation to choose direct dictionary lookups or null lookups when possible instead of always using the R2R helper — is intentional and correct.


Detailed Findings

Detailed Findings

✅ Correctness — Behavioral equivalence verified

The old code always forced the R2R helper path:

  • needsRuntimeLookup = true
  • indirections = USEHELPER
  • helper = CORINFO_HELP_READYTORUN_GENERIC_HANDLE

ComputeLookup (lines 253–316) first calls _compilation.NeedsRuntimeLookup(helperId, entity), then _compilation.ComputeGenericLookup(callerHandle, helperId, entity) to determine the optimal strategy. When UseHelper is true, it produces the same R2R helper setup. When a more efficient path is available (direct dictionary lookup or null), it uses that instead. This is the intended improvement per the PR title.

The pResolvedToken.tokenContext != contextFromMethodBeingCompiled() guard (line 272–276) also correctly handles inlining scenarios by returning CORINFO_LOOKUP_NOT_SUPPORTED, which the old inline code did not account for.

✅ Consistency — Matches established pattern

All other runtime-lookup call sites in getCallInfo already use ComputeLookup:

  • Line 1437: ReadyToRunHelperId.MethodEntry (constrained call)
  • Line 1577: ReadyToRunHelperId.ConstrainedDirectCall
  • Line 1600: ReadyToRunHelperId.MethodHandle
  • Line 1618: ReadyToRunHelperId.VirtualDispatchCell

The changed block now matches these exactly.

💡 Cross-cutting observation — Other inline lookup sites

Two other call sites in the same file still use the inline GetGenericRuntimeLookupKind + GetGenericLookupHelper pattern (line 349 in getReadyToRunHelper and line 445 in getReadyToRunDelegateCtorHelper). These use different helper IDs (GetNonGCStaticBase and DelegateCtor respectively) so they may have different constraints, but could potentially benefit from a similar refactoring in the future.

Note

This review was created by GitHub Copilot.

Generated by Code Review for issue #129560 · ● 37.6M ·

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

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants