-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix issue with array interface devirtualization #120313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The instantiated methods should use the parameter type of the interface, not the element type of the array. Fixes dotnet#120270.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fixes an issue with array interface devirtualization where the JIT was incorrectly using the array element type instead of the interface parameter type when instantiating methods. This caused failures during array interface method calls when the array element type was canonical but the interface type was not.
Key Changes:
- Updated devirtualization logic to use interface parameter type instead of array element type
- Added regression test to verify the fix works correctly
- Removed unnecessary canonical subtype check since interface type is guaranteed to not be canonical
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/coreclr/vm/jitinterface.cpp |
Fixed devirtualization logic to use interface parameter type instead of array element type |
src/tests/JIT/Regression/JitBlue/Runtime_120270/Runtime_120270.cs |
Added regression test that reproduces the original issue |
src/tests/JIT/Regression/JitBlue/Runtime_120270/Runtime_120270.csproj |
Project file for the regression test |
|
@davidwrighton PTAL |
|
Was hitting almost the same issue when I tried to enable devirt for GVM as well: https://github.com/dotnet/runtime/pull/112353/files#diff-98b4e379205d747e922689cb993f985be190fedb509fd486e99280c842703472R8786 |
|
/azp run runtime-coreclr outerloop, runtime-coreclr pgo |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
This is going to regress deabstraction for enumeration of arrays of reference types, eg
Seems like to handle this we now need to profile both the object type and the interface type as a pair, and GDV for both, or something like that. Or maybe just GDV for the object type and then add a runtime check that the interface type is the same as the array element type? Need to look at this some more but seems possible there's no simple way to avoid taking this hit. |
|
@davidwrighton for ref types wouldn't normal variance rules apply? So using the element type there is ok? |
|
@AndyAyersMS unfortunately... a string[] will expose both |
|
Here's an example showing us switching ref array enumerator types after optimizing: class Base {}
class Derived : Base {}
class Problem
{
public static void Test()
{
for (int i = 0; i < 100_000; i++)
{
Derived[] d = [new Derived()];
IEnumerable<Base> e = d;
IEnumerator<Base> en = e.GetEnumerator();
if (i % 20_000 == 0)
{
Console.WriteLine(en.GetType());
}
}
}
}Running this prints with the switch happening after OSR kicks in. |
|
@davidwrighton updated to block devirt if interface type is _Canon. |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/18230560856 |
The instantiated methods should use the parameter type of the interface, not the element type of the array.
Fixes #120270.