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

[mono][aot] Enabling AOT wrappers for virtual delegates #85643

Merged
merged 21 commits into from
May 22, 2023
Merged

[mono][aot] Enabling AOT wrappers for virtual delegates #85643

merged 21 commits into from
May 22, 2023

Conversation

LeVladIonescu
Copy link
Contributor

@LeVladIonescu LeVladIonescu commented May 2, 2023 •
edited
Loading

Adding AOT wrappers for delegate invoke virtual.
Contributes to #83329.

Vlad - Alexandru Ionescu added 5 commits April 26, 2023 20:31
try to add virt delegates
33604b1
Added encode and decode of klass for DELEGATE_VIRTUAL subtype
e0d201d 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
Changed how construction of callvirt delegate is done
6b6dae9 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
Encode class only for delegate_invoke_virtual subtype
c98a748 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
Added condition for encoding wrapper subtype
0dbe14f
@LeVladIonescu LeVladIonescu added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 2, 2023
@ghost ghost assigned LeVladIonescu May 2, 2023
LeVladIonescu
LeVladIonescu commented May 2, 2023
View reviewed changes
src/mono/mono/mini/aot-compiler.c Outdated
@@ -3778,6 +3778,8 @@ encode_method_ref (MonoAotCompile *acfg, MonoMethod *method, guint8 *buf, guint8
MonoJumpInfoToken *ji;
guint8 *p = buf;

printf("NAME = %s\n", method->name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unrelated change...only for debug reasons

@LeVladIonescu LeVladIonescu requested a review from marek-safar as a code owner May 4, 2023 12:52
@build-analysis build-analysis bot mentioned this pull request May 4, 2023
Closed
Inst generic types for delegates
01231ce 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
@LeVladIonescu LeVladIonescu changed the title [DRAFT] Enabling AOT wrappers for virtual delegates [mono][aot] Enabling AOT wrappers for virtual delegates May 10, 2023
@LeVladIonescu LeVladIonescu removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 10, 2023
@build-analysis build-analysis bot mentioned this pull request May 10, 2023
Closed
LeVladIonescu and others added 3 commits May 11, 2023 10:05
@LeVladIonescu
Copy link
Contributor Author

Ready to review / merge.
Library tests build failures from CI are related.

@LeVladIonescu
Copy link
Contributor Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request May 12, 2023
Closed
@runfoapp runfoapp bot mentioned this pull request May 15, 2023
Closed
@SamMonoRT
Copy link
Member

@LeVladIonescu - as part of this PR consider enabling any disabled tests fixed by the changes.

@LeVladIonescu
Copy link
Contributor Author

@SamMonoRT will check

kotlarmilos
kotlarmilos reviewed May 16, 2023
View reviewed changes
src/mono/mono/mini/aot-compiler.c
shared_context = mono_class_get_generic_container (method->klass)->context;
inst = shared_context.class_inst;

args = g_new0 (MonoType*, inst->type_argc);
Copy link
Member

Choose a reason for hiding this comment

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

Is the memory being deallocated correctly?

Copy link
Contributor Author

@LeVladIonescu LeVladIonescu May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, it is not. I would suggest deallocating it after

runtime/src/mono/mono/metadata/metadata.c

Line 3399 in 175dae9

memcpy (ginst->type_argv, type_argv, type_argc * sizeof (MonoType *));

^ @vargaz

Copy link
Contributor

Choose a reason for hiding this comment

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

I can be freed, but the aot compiler is already leaking memory in a number of places, so its not that important.

Moved signature change to use generic version of the wrapper and fixe…
3658fbc 
…d indentation

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
@LeVladIonescu
Copy link
Contributor Author

Current failures are known and seem unrelated:

@LeVladIonescu
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced May 18, 2023
Open
Closed
@LeVladIonescu
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

LeVladIonescu and others added 2 commits May 19, 2023 00:26
@LeVladIonescu
Merge branch 'dotnet:main' into AOT_delegates
9f4788c
Deallocate args
26bf6bf 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
@LeVladIonescu
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Added check for primitives
3e4c73d 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
@LeVladIonescu
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced May 21, 2023
Closed
Closed
@LeVladIonescu
Copy link
Contributor Author

Remaining failures are known and unrelated.

Remove commented line
d44a7ae 
Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
@LeVladIonescu LeVladIonescu merged commit e91db04 into dotnet:main May 22, 2023
@LeVladIonescu LeVladIonescu deleted the AOT_delegates branch May 22, 2023 10:35
This was referenced May 22, 2023
Closed
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kotlarmilos kotlarmilos kotlarmilos left review comments

@vargaz vargaz vargaz approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@LeVladIonescu LeVladIonescu

Labels
area-Codegen-AOT-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LeVladIonescu @SamMonoRT @vargaz @kotlarmilos