Skip to content

Conversation

@davidwrighton
Copy link
Member

  • The implementation uses the true LDVIRTFTN path for this. Since it thunks to using function pointer the perf is likely not ideal, but we will need a new generic dictionary entry kind to make this simple, and I'd like to avoid making that change to unblock progress here
  • This expensive path was probably fine for use with virtual generic methods, but regular interface dispatch shouldn't be this expensive, so we'll want to fix this later

- The implementation uses the true LDVIRTFTN path for this. Since it thunks to using function pointer the perf is likely not ideal, but we will need a new generic dictionary entry kind to make this simple, and I'd like to avoid making that change to unblock progress here
- This expensive path was probably fine for use with virtual generic methods, but regular interface dispatch shouldn't be this expensive, so we'll want to fix this later
Copilot AI review requested due to automatic review settings July 1, 2025 17:34
@davidwrighton davidwrighton requested a review from kotlarmilos July 1, 2025 17:34
@dotnet-policy-service
Copy link
Contributor

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

Copy link
Contributor

Copilot AI left a comment

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 enables calls to shared generic interfaces within shared generic code by routing interface dispatch through the LDVIRTFTN path.

  • Implements IGeneric<T> on GenericClass<T> with an explicit Method returning typeof(T)
  • Adds a test invocation of the new interface path in TestGenerics_CallsFrom<T>()
  • Updates the JIT interpreter (compiler.cpp) to include shared-instantiated classes in the CORINFO_VIRTUALCALL_LDVIRTFTN path

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/tests/JIT/interpreter/Interpreter.cs Added IGeneric<T> interface; implemented IGeneric<T>.Method in GenericClass<T>; exercised interface dispatch in TestGenerics_CallsFrom<T>()
src/coreclr/interpreter/compiler.cpp Extended the CORINFO_VIRTUALCALL_LDVIRTFTN case to treat shared-instantiated classes as virtual calls via ldvirtftn
Comments suppressed due to low confidence (2)

src/tests/JIT/interpreter/Interpreter.cs:2143

  • The shared-generic interface dispatch path isn’t covered in TestSharedGenerics_CallsTo<T>(). Adding a cast to IGeneric<T> and invoking Method() there would ensure this code path is exercised under shared generics.
    interface IGeneric<T>

src/tests/JIT/interpreter/Interpreter.cs:1899

  • [nitpick] The interface method name Method is generic and may be unclear; consider renaming it to something like GetTypeOfT to improve readability.
        Type IGeneric<T>.Method()

@davidwrighton davidwrighton enabled auto-merge (squash) July 1, 2025 22:46
@davidwrighton davidwrighton merged commit e4bbf7e into dotnet:main Jul 2, 2025
102 of 104 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants