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

Mono: devirtualize sealed calls #33015

Closed
EgorBo opened this issue Feb 29, 2020 · 3 comments
Closed

Mono: devirtualize sealed calls #33015

EgorBo opened this issue Feb 29, 2020 · 3 comments
Assignees
Milestone

Comments

@EgorBo
Copy link
Member

EgorBo commented Feb 29, 2020

Mono doesn't devirtualize (and inline) sealed methods, e.g.:

public class A 
{
    public virtual int Foo() => 1;
}

public sealed class B : A
{
    public sealed override int Foo() => 2;  // sealed!
}

...
static int Test(B b)
{
    return b.Foo(); // Foo should be devirtualized and inlined
}

b.Foo() is expected to be devirtualized and inlined because it is sealed.

Current codegen:

loWorld_Program_Test__HelloWorld_B_:
0000000000000000	pushq	%rax
0000000000000001	movq	(%rdi), %rax
0000000000000004	callq	*0x68(%rax)
0000000000000007	popq	%rcx
0000000000000008	retq
0000000000000009	movabsq	$0x7fd559c02530, %rax
0000000000000013	movl	$0x120, %edi
0000000000000018	callq	*(%rax)

Expected codegen:

loWorld_Program_Test__HelloWorld_B_:
0000000000000000	movl	$0x2, %eax  ;; also: nullcheck
0000000000000005	retq

CoreCLR is able to do it, part of the jitdump for Test:


impDevirtualizeCall: Trying to devirtualize virtual call:
    class for 'this' is ConsoleApp4.B [final] (attrib 20000010)
    base method is ConsoleApp4.A::Foo
    devirt to ConsoleApp4.B::Foo -- final class
               [000001] --C-G-------              *  CALLV ind int    ConsoleApp4.A.Foo
               [000000] ------------ this in rcx  \--*  LCL_VAR   ref    V00 arg0         
    final class; can devirtualize
... after devirt...
               [000001] --C-G-------              *  CALL nullcheck int    ConsoleApp4.B.Foo
               [000000] ------------ this in rcx  \--*  LCL_VAR   ref    V00 arg0    
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added untriaged New issue has not been triaged by the area owner area-CodeGen labels Feb 29, 2020
@EgorBo EgorBo added area-Codegen-meta-mono and removed area-CodeGen untriaged New issue has not been triaged by the area owner labels Feb 29, 2020
@EgorBo
Copy link
Member Author

EgorBo commented Feb 29, 2020

Also, Xamarin.iOS has a linker substep where it marks all methods and classes as sealed if there are no subclasses/overrides.
So this opt will be nice to have.

@omariom
Copy link
Contributor

omariom commented Feb 29, 2020

it marks all methods and classes as sealed if there are no subclasses/overrides.

@MichalStrehovsky Does CoreRT do the same?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky Does CoreRT do the same?

Yes, and it's behind policies so that we don't do this if Assembly.Load or Reflection.Emit is allowed.

@lambdageek lambdageek added this to the 6.0.0 milestone Jun 22, 2020
@SamMonoRT SamMonoRT modified the milestones: 6.0.0, 7.0.0 Jun 16, 2021
@SamMonoRT SamMonoRT modified the milestones: 7.0.0, 8.0.0 Aug 3, 2022
jandupej added a commit to jandupej/runtime that referenced this issue Sep 16, 2022
jandupej added a commit to jandupej/runtime that referenced this issue Sep 21, 2022
jandupej added a commit that referenced this issue Oct 12, 2022
* [mono][jit] Devirtualizing ldarg(s)+callvirt where possible. #33015

* [mono][jit] Resolved compiler warnings. #33015

* [mono][jit] Fixed runtime crash when optimizing ldarg(a)+callvirt and vtable is unavailable. #33015

* [mono][jit] Checking mono_class_get_virtual_method assumptions now also fails when klass is abstract.

* [mono][jit] Cleaned up optimization of ldarg(a)+callvirt. GetHashCode is devirtualized in callvirt handler now.

* [mono][jit] ldarg(a)+callvirt optimization code style now conforms to guidelines.

* [mono][jit] Simplified conditions on ldarg(a)+callvirt optimization. Fixed code style issues.

* [mono][jit] Indentation fix. Vtable is now set up in check_get_virtual_method_assumptions.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants