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][jit] Devirtualize ldarg(s)+callvirt where possible. #75748

Merged
merged 10 commits into from
Oct 12, 2022

Conversation

jandupej
Copy link
Member

@jandupej jandupej commented Sep 16, 2022

This addresses Mono:devirtualize sealed calls. If ldarg(a) is succeeded by a callvirt, we devirtualize that call if we can resolve to a final method at that time. Additional conditions apply, mainly no AOT and no generics. The method also must have no arguments except this.

The example from the said issue is now devirtualized and inlined. The codegen for Test(B b) is as follows on arm64:

loWorld_Program_Test__HelloWorld_B_:
0000000000000000        stp     x29, x30, [sp, #-0x20]!
0000000000000004        mov     x29, sp
0000000000000008        str     x26, [x29, #0x10]
000000000000000c        mov     x26, x0
0000000000000010        ldrb    w30, [x26]
0000000000000014        mov     x0, #0x2    ; return 2 from B:Foo
0000000000000018        ldr     x26, [x29, #0x10]
000000000000001c        mov     sp, x29
0000000000000020        ldp     x29, x30, [sp], #0x20
0000000000000024        ret

Given:

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
}

@jandupej jandupej added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Sep 16, 2022
@ghost ghost assigned jandupej Sep 16, 2022
@jandupej jandupej marked this pull request as draft September 16, 2022 10:23
@jandupej
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jandupej jandupej removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Sep 27, 2022
@jandupej
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jandupej
Copy link
Member Author

jandupej commented Oct 5, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Oct 6, 2022

the wasm build test failure was #76621

and should be fixed now

@vargaz
Copy link
Contributor

vargaz commented Oct 7, 2022

Looks ok.

try_prepare_objaddr_callvirt_optimization (MonoCompile *cfg, guchar *next_ip, guchar* end, MonoMethod *method, MonoGenericContext* generic_context, MonoClass *klass)
{
// TODO: relax the _is_def requirement?
if (cfg->compile_aot || cfg->compile_llvm || !klass || !mono_class_is_def (klass))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these can be relaxed later.


if (m_class_get_vtable (klass) == NULL)
return FALSE;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen if the vtable is not created yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a vtable setup, as in the regular mono_class_get_virtual_method.

@jandupej jandupej marked this pull request as ready for review October 7, 2022 11:24
@jandupej jandupej merged commit f2de14d into dotnet:main Oct 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2022
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.

4 participants