Skip to content
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

JIT: GDV does not handle delegates pointing to generic methods correctly #89495

Closed
jakobbotsch opened this issue Jul 26, 2023 · 1 comment · Fixed by #89499
Closed

JIT: GDV does not handle delegates pointing to generic methods correctly #89495

jakobbotsch opened this issue Jul 26, 2023 · 1 comment · Fixed by #89499
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Jul 26, 2023

For delegates, we currently retain the context from the IL (similarly to how we used to do it for type-based GDV before #87847). For example:

[MethodImpl(MethodImplOptions.NoInlining)]
static int Foo(Func<int, int> test)
{
    return test(3);
}

public static int Main()
{
    for (int i = 0; i < 100; i++)
    {
        Foo(new Runtime_87597().Id<int>);

        if (i >= 30)
        {
            Thread.Sleep(10);
        }
    }

    return 100;
}

private T Id<T>(T val) => val;

results in

Importing BB01 (PC=000) of 'Runtime_87597:Foo(System.Func`2[int,int]):int'
    [ 0]   0 (0x000) ldarg.0
    [ 1]   1 (0x001) ldc.i4.3 3
    [ 2]   2 (0x002) callvirt 0A000001
 (Implicit Tail call: prefixFlags |= PREFIX_TAILCALL_IMPLICIT)
In Compiler::impImportCall: opcode is callvirt, kind=0, callRetType is int, structSize is 0
Considering guarded devirtualization at IL offset 2 (0x2)
Likely methods for call [000002] to method System.Func`2[int,int]:Invoke(int):int:this
  1) 00007FFAF0A1B5E8 (Runtime_87597:Id[int](int):int:this) [likelihood:100%]
delegate call would invoke method Runtime_87597:Id[int](int):int:this
Marking call [000002] as guarded devirtualization candidate; will guess for method Runtime_87597:Id[int](int):int:this

info.compCompHnd->canTailCall returned false for call [000002]

CheckCanInline: fetching method info for inline candidate Id -- context 00007FFAF0A63C99
Class context: System.Func`2[int,int]
INLINER: during 'impMarkInlineCandidate for GDV' result 'CheckCanInline Success' reason 'CheckCanInline Success' for 'Runtime_87597:Foo(System.Func`2[int,int]):int' calling 'System.Func`2[int,int]:Invoke(int):int:this'
INLINER: during 'impMarkInlineCandidate for GDV' result 'CheckCanInline Success' reason 'CheckCanInline Success'

The Class context: System.Func`2[int,int] is not right. This is presumably a correctness issue similar to #87847.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 26, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2023
@ghost
Copy link

ghost commented Jul 26, 2023

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

Issue Details

For delegates, we currently retain the context from the IL (similarly to how we used to do it for delegate GDV before #87847). For example:

[MethodImpl(MethodImplOptions.NoInlining)]
static string Foo(Func<string, string> test)
{
    return test("abc");
}

public static int Main()
{
    for (int i = 0; i < 100; i++)
    {
        Foo(new Runtime_87597().Id<string>);

        if (i >= 30)
        {
            Thread.Sleep(10);
        }
    }

    return 100;
}

private T Id<T>(T val) => val;

results in

Importing BB01 (PC=000) of 'Runtime_87597:Foo(System.Func`2[System.String,System.String]):System.String'
    [ 0]   0 (0x000) ldarg.0
    [ 1]   1 (0x001) ldstr 70000001
    [ 2]   6 (0x006) callvirt 0A000001
 (Implicit Tail call: prefixFlags |= PREFIX_TAILCALL_IMPLICIT)
In Compiler::impImportCall: opcode is callvirt, kind=0, callRetType is ref, structSize is 0
Considering guarded devirtualization at IL offset 6 (0x6)
Likely methods for call [000002] to method System.Func`2[System.__Canon,System.__Canon]:Invoke(System.__Canon):System.__Canon:this
  1) 00007FFAF0DDB600 (Runtime_87597:Id[System.String](System.String):System.String:this) [likelihood:100%]
delegate call would invoke method Runtime_87597:Id[System.String](System.String):System.String:this
Marking call [000002] as guarded devirtualization candidate; will guess for method Runtime_87597:Id[System.String](System.String):System.String:this

info.compCompHnd->canTailCall returned false for call [000002]

CheckCanInline: fetching method info for inline candidate Id -- context 00007FFAF0E23EE9
Class context: System.Func`2[System.String,System.String]
INLINER: during 'impMarkInlineCandidate for GDV' result 'failed this callee' reason 'cannot get method info' for 'Runtime_87597:Foo(System.Func`2[System.String,System.String]):System.String' calling 'System.Func`2[System.__Canon,System.__Canon]:Invoke(System.__Canon):System.__Canon:this'

INLINER: Marking System.Func`2[System.__Canon,System.__Canon]:Invoke(System.__Canon):System.__Canon:this as NOINLINE because of cannot get method info
INLINER: during 'impMarkInlineCandidate for GDV' result 'failed this callee' reason 'cannot get method info'

Beyond just resulting in us not being able to inline it presumably also is a correctness issue similar to #87847, though I'm not familiar enough with that problem to construct a test case.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jul 26, 2023
Instead of passing through the IL context, update the method context to
be the exact method that were recorded in the delegate (which will
include the full instantiation).

Also do a bit of clean up.

Fix dotnet#89495
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 26, 2023
@jakobbotsch jakobbotsch self-assigned this Jul 26, 2023
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Jul 26, 2023
@jakobbotsch jakobbotsch added this to the 8.0.0 milestone Jul 26, 2023
jakobbotsch added a commit that referenced this issue Jul 27, 2023
Instead of passing through the IL context, update the method context to
be the exact method that were recorded in the delegate (which will
include the full instantiation).

Also do a bit of clean up.

Fix #89495
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant