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.

Sign up for GitHub

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

Jump to bottom

JIT cannot devirtualize interface call in trivial cases #63283

Closed
sakno opened this issue Jan 3, 2022 · 11 comments
Closed

JIT cannot devirtualize interface call in trivial cases #63283

sakno opened this issue Jan 3, 2022 · 11 comments
Assignees
@AndyAyersMS
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue

Comments

@sakno
Copy link
Contributor

sakno commented Jan 3, 2022 •
edited
Loading

Description

JIT cannot devirtualize interface call in very trivial cases.

Source code:

using System;
using System.Runtime.CompilerServices;

public static class C {
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T As<T>(T obj) where T : class => obj;
    
    public static int Cmp(String str) => As<IComparable<string>>(str).CompareTo(string.Empty);
    
    public static int Cmp2(String str) => str.CompareTo(string.Empty);
    
    public static int Cmp3(String str) => ((IComparable<string>)str).CompareTo(string.Empty);
    
    public static int Cmp4(IComparable<string> str) => str.CompareTo(string.Empty);
}

Output assembly:

C.Cmp(System.String)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317000]
    L000f: pop ebp
    L0010: ret

C.Cmp2(System.String)
    L0000: mov edx, [0x8d72018]
    L0006: cmp [ecx], ecx
    L0008: push 0
    L000a: call System.String.Compare(System.String, System.String, System.StringComparison)
    L000f: ret

C.Cmp3(System.String)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317004]
    L000f: pop ebp
    L0010: ret

C.Cmp4(System.IComparable`1<System.String>)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317008]
    L000f: pop ebp
    L0010: ret

Cmp2 is correctly devirtualized and inlined. I'm expecting that the assembly code should be the same for all cases except Cmp4.

Configuration

.NET 6.0.101
Ubuntu 20.04.3 5.4.0-91-generic x86_64

Regression?

Probably yes, but I didn't check.
@sakno sakno added the tenet-performance Performance related issue label Jan 3, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Jan 3, 2022
@ghost
Copy link

ghost commented Jan 3, 2022

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

Issue Details

Description

JIT cannot devirtualize interface call in very trivial cases.

Source code:

using System;
using System.Runtime.CompilerServices;

public static class C {
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static T As<T>(T obj) where T : class => obj;
    
    public static int Cmp(String str) => As<IComparable<string>>(str).CompareTo(string.Empty);
    
    public static int Cmp2(String str) => str.CompareTo(string.Empty);
    
    public static int Cmp3(String str) => ((IComparable<string>)str).CompareTo(string.Empty);
    
    public static int Cmp4(IComparable<string> str) => str.CompareTo(string.Empty);
}

Output assembly:

C.Cmp(System.String)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317000]
    L000f: pop ebp
    L0010: ret

C.Cmp2(System.String)
    L0000: mov edx, [0x8d72018]
    L0006: cmp [ecx], ecx
    L0008: push 0
    L000a: call System.String.Compare(System.String, System.String, System.StringComparison)
    L000f: ret

C.Cmp3(System.String)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317004]
    L000f: pop ebp
    L0010: ret

C.Cmp4(System.IComparable`1<System.String>)
    L0000: push ebp
    L0001: mov ebp, esp
    L0003: mov edx, [0x8d72018]
    L0009: call dword ptr [0x11317008]
    L000f: pop ebp
    L0010: ret

Cmp2 is correctly devirtualized. I'm expecting that the assembly code should be the same for all cases except Cmp4.

Configuration

.NET 6.0.101
Ubuntu 20.04.3 5.4.0-91-generic x86_64

Regression?

Probably yes, but I didn't check.

Author: sakno
Assignees: -
Labels:

tenet-performance, area-CodeGen-coreclr, untriaged
Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Jan 3, 2022

Cmp2 and Cmp3 have the same codegen in .NET 7.0
Cmp1 is a known JIT's limitation - we don't re-visit calls we marked as non-devirtualizable/non-inlinineable candidates after we inlined some inner calls in the same method.

But it does happen when Dynamic PGO is turned on.

@sakno
Copy link
Contributor Author

sakno commented Jan 3, 2022

Any chance to backport JIT behavior as you described to .NET 6 ?

@EgorBo
Copy link
Member

EgorBo commented Jan 3, 2022

Any chance to backport JIT behavior as you described to .NET 6 ?

Dynamic PGO? you can enable it in .NET 6.0, see https://gist.github.com/EgorBo/dc181796683da3d905a5295bfd3dd95b#how-can-i-try-it-on-my-production

@sakno
Copy link
Contributor Author

sakno commented Jan 3, 2022

No, not Dynamic PGO. I'm talking about to have the same codegen for Cmp2 and Cmp3 in .NET 6.

@EgorBo
Copy link
Member

EgorBo commented Jan 3, 2022

No, not Dynamic PGO. I'm talking about to have the same codegen for Cmp2 and Cmp3 in .NET 6.

Unlikely, the bar to backport changes to .NET 6.0 is very high to include an optimization.

@AndyAyersMS
Copy link
Member

I think we should be able to get the Cmp case via late devirtualization, but for some reason the interface class we end up with doesn't seem to be compatible with string and so we fail to do so now. Need to analyze further.

*** CMP

impDevirtualizeCall: Trying to devirtualize interface call: (late)

    class for 'this' is System.String [final] (attrib 20040010)
    base method is System.IComparable`1[__Canon]::CompareTo
--- base class is interface
--- no derived method: object class could not be cast to interface class
    Class not final or exact
No guarded devirt during late devirtualization

*** CMP3

impDevirtualizeCall: Trying to devirtualize interface call: (early)
    class for 'this' is System.String [final] (attrib 20040010)
    base method is System.IComparable`1[__Canon]::CompareTo
--- base class is interface
    devirt to System.String::CompareTo -- final class

@AndyAyersMS
Copy link
Member

Seems like the issue is that in the Cmp case we end up losing the generic context for the As (which should be somehow associated with the return value) and so are trying to cast from System.String to IComparable<_Canon> sans context, and this fails in the runtime, whereas with Cmp3 we have appropriate context and are casting to IComparable<System.String>.

When we propagate a return value out of a generic method to the non-generic root, we ideally should be able to determine the proper unshared type for that value. This might work roughly as follows:

  • in impReturnInstruction, if the inlinee has a generics context, record it on a new field in the GT_RET_EXPR
  • when swapping out the GT_RET_EXPR for the return value tree, somehow attach the context to the return value tree. if we're updating the this of virtual or interface call. Note we could attach it to the parent call perhaps.... would need to be a bit careful as this is just the context for this and not for the body of the method. But since we can't inline after this, perhaps it would be ok.
  • then pass this context to the call to impDevirtualizeCall we make in fgLateDevirtualization.

@AndyAyersMS
Copy link
Member

Another perhaps more robust option would be to retype the return value, if it's a ref type, and we have a context.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 5, 2022
@AndyAyersMS
JIT: save generics context for late devirtualization
f43fd40 
If we have a non-inline candidate call with generics context, save
the context so it's available for late devirtualization.

Fixes a missing devirtualization reported in dotnet#63283.

I am deliberately leaving `LateDevirtualizationInfo` more general than
necessary as it may serve as a jumping-off point for enabling late
inlining.
@AndyAyersMS AndyAyersMS mentioned this issue Jan 5, 2022
Merged
@AndyAyersMS AndyAyersMS self-assigned this Jan 6, 2022
@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Jan 6, 2022
@AndyAyersMS
Copy link
Member

Fix for this in PR #63420.

AndyAyersMS added a commit that referenced this issue Jan 6, 2022
@AndyAyersMS
JIT: save generics context for late devirtualization (#63420)
a0220fd 
If we have a non-inline candidate call with generics context, save
the context so it's available for late devirtualization.

Fixes a missing devirtualization reported in #63283.

I am deliberately leaving `LateDevirtualizationInfo` more general than
necessary as it may serve as a jumping-off point for enabling late
inlining.
@sakno
Copy link
Contributor Author

sakno commented Jan 7, 2022

Closing issue as PR has been merged.

@sakno sakno closed this as completed Jan 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@AndyAyersMS AndyAyersMS

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@EgorBo @sakno @AndyAyersMS