[DRAFT][mono][aot] Implementation of nollvm init method #89074

LeVladIonescu · 2023-07-18T10:30:00Z

This PR aims to introduce the implementation of method's self initialization for nollvm cases, firstly on Arm64.
This won't be enabled since it's not tested yet.

Addresses #82224.

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MBP-5.guest.corp.microsoft.com>

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

LeVladIonescu · 2023-07-18T11:05:35Z

@vargaz @lambdageek This is ready for review, I've enabled the optimization just to see how it behaves on CI.
cc @kotlarmilos

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

LeVladIonescu · 2023-07-18T13:29:39Z

src/mono/mono/mini/aot-runtime.c

+			mono_bitset_set (inited_bitset, method_index);
+		}
+	}
+	mono_error_assert_ok (error);


@vargaz do we want to propagate this error?

lambdageek

I'm not sure I completely followed what is going on. but the marshal stuff looks ok. I had some minor suggestions for method-to-ir and the arm64 backend. And a question about why this is backend-specific.

lambdageek · 2023-07-19T02:51:08Z

src/mono/mono/mini/method-to-ir.c

+		case MONO_CEE_AOT_MODULE: {
+			if (cfg->compile_aot) {
+				EMIT_NEW_AOTCONST (cfg, ins, MONO_PATCH_INFO_AOT_MODULE, NULL);
+				*sp++ = ins;
+			}
+			break;
+		}


We don't expect to see this opcode if we're JITing, right? can we do g_assert (cfg->compile_aot)?
Alternately, can we add an else case that pushes some dummy constant (NULL?) on the stack. That way even if we emit this during JITing by accident it will just mean we'll see a NULL module in the icall instead of getting a messed up operand stack in the wrapper method.

I think I'll go with the first approach, since the second will also need a NULL check for the amodule inside the call.

lambdageek · 2023-07-19T02:57:06Z

src/mono/mono/mini/mini-arm64.c

+emit_call (MonoCompile *cfg, guint8* code, MonoJumpInfoType patch_type, gconstpointer data, MonoMethod *method)
 {
 	/*
 	mono_add_patch_info_rel (cfg, code - cfg->native_code, patch_type, data, MONO_R_ARM64_IMM);
 	code = emit_imm64_template (code, ARMREG_LR);
 	arm_blrx (code, ARMREG_LR);
 	*/
-	mono_add_patch_info_rel (cfg, code - cfg->native_code, patch_type, data, MONO_R_ARM64_BL);
+	mono_add_patch_info_rel (cfg, code - cfg->native_code, patch_type, method ? method : data, MONO_R_ARM64_BL);


This seems unnecessary - MonoMethod* is a pointer-sized value. You can just pass it in for the data argument (possibly with a cast to deal with the const-ness).

src/mono/mono/mini/mini-arm64.c

src/mono/mono/metadata/marshal.c

src/mono/mono/mini/aot-compiler.c

kotlarmilos · 2023-07-19T09:29:25Z

src/mono/mono/mini/aot-runtime.c

+	/*
+	 * Methods init bitset used for initialization during the runtime
+	 */
+	amodule->mono_inited = mono_bitset_new (amodule->info.nmethods, 0);


As we discussed offline, would be good to think of memory leaks. Is it allocated within the mempool?

LeVladIonescu · 2023-07-19T14:41:33Z

I'm not sure I completely followed what is going on. but the marshal stuff looks ok. I had some minor suggestions for method-to-ir and the arm64 backend. And a question about why this is backend-specific.

The main idea is to have this aot_get_init_wrapper being called from the method's prolog which will check (for nollvm cases) if the method is already initialised or not. If it is not, then a callback and a call will be used to call init_method (i.e. mini_nollvm_init_method).
This is targeting only arm64 at the moment due to testing purposes, the plan is to enable it on all platforms.

lambdageek · 2023-07-19T15:35:00Z

I'm not sure I completely followed what is going on. but the marshal stuff looks ok. I had some minor suggestions for method-to-ir and the arm64 backend. And a question about why this is backend-specific.

The main idea is to have this aot_get_init_wrapper being called from the method's prolog which will check (for nollvm cases) if the method is already initialised or not. If it is not, then a callback and a call will be used to call init_method (i.e. mini_nollvm_init_method). This is targeting only arm64 at the moment due to testing purposes, the plan is to enable it on all platforms.

got it. thanks. are we planning to use this by default? or only if we enable some direct call aot option? This is introducing slow overhead on every method call, right?

If I call _SomeAssembly_SomeClass_SomeMethod() the first thing it will do every time is bl _aot_init_wrapper right? So if I call SomeMethod in a loop 10000 times, we will call the init wrapper 10000 times and after the first call it will always just immediately return?

For a follow-up PR, is there some way we can emit an inlined check of the bitmask? so that in the fast path we're just doing a memory load and avoiding a call from the prolog of every method?

vargaz · 2023-07-19T16:18:14Z

So this is an experiment to see whenever this can be made to work and what the perf impact woudl be. In theory, it can be made as fast as the llvm version, which does:
if (!bitmap[idx]) init method (idx)
The init method should also have the 'cold' calling convention in the future, which means it doesn't clobber any registers, beside the one used to pass idx.

The upside is that these methods can now be called directly, the caller doesn't have to go through a PLT entry. So the method becomes a little slower, but calling it becomes a little cheaper.

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

lambdageek

Looking good. I'm not sure about ENABLE_LLVM - that just means whether the AOT compiler has LLVM support built-in - not that the LLVM support is being used for the current compilation - so I'm not sure that's the condition for deciding what does into the wrapper method.

src/mono/mono/mini/aot-compiler.c

src/mono/mono/mini/mini-arm64.c

src/mono/mono/mini/aot-compiler.c

lambdageek · 2023-07-21T15:07:56Z

src/mono/mono/mini/aot-runtime.c

+#ifdef ENABLE_WIP_METHOD_NOLLVM_SELF_INIT
+			else 
+				mono_bitset_set (mono_aot_get_mono_inited(amodule), method_index);
+#endif


you used to have an arm64 target check here (and also that LLVM was disabled). but now you don't.

Do you need to enable ENABLE_WIP_METHOD_NOLLVM_SELF_INIT in src/mono/CMakeLists.txt if we're targeting arm64?

lambdageek · 2023-07-21T15:15:24Z

src/mono/mono/metadata/marshal.c

+#ifndef ENABLE_LLVM
+	get_marshal_cb ()->emit_method_init (mb);
+#endif


This doesn't make sense to me. ENABLE_LLVM just means we're building the runtime with LLVM support. It doesn't mean that we're using LLVM for the current AOT compilation. I think you need two different wrapper subtypes AOT_INIT_METHOD_LLVM and AOT_INIT_METHOD_NOLLVM and at the callsite you would do something like mono_marshal_get_aot_init_wrapper (cfg->compile_aot && COMPILE_LLVM(cfg) ? AOT_INIT_METHOD_LLVM : AOT_INIT_METHOD_NOLLVM)

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

Signed-off-by: Vlad <vladionescu1399@gmail.com>

LeVladIonescu · 2023-08-17T21:36:57Z

Converting this PR to draft in order to test experimental changes on arm64.
These action points need to be solved in order to merge it first for arm64:

make it work on the sample app
runtime working properly with everything inited
CI green

cc @kotlarmilos @SamMonoRT

kotlarmilos · 2023-08-18T15:47:49Z

src/mono/mono/mini/mini-arm64.c

+#ifdef ENABLE_WIP_METHOD_NOLLVM_SELF_INIT
+	if (cfg->compile_aot && !COMPILE_LLVM (cfg)) {
+		code = emit_imm (code, ARMREG_R0, cfg->method_index);
+		code = emit_call (cfg, code, MONO_PATCH_INFO_METHOD, (gconstpointer) mono_marshal_get_aot_init_wrapper (AOT_INIT_METHOD));


I am not sure if this is a correct way to invoke wrappers generated in:

runtime/src/mono/mono/mini/aot-compiler.c

Line 4470 in 6229f5f

add_method (acfg, mono_marshal_get_aot_init_wrapper ((MonoAotInitSubtype)i));

During the native linking, there is an undefined symbols for architecture arm64:

"_mono_aot_HelloWorld_init_method", referenced from: HelloWorld_Program_Main_string__ in mono_aot_0nLeOF.o HelloWorld_Program__ctor in mono_aot_0nLeOF.o wrapper_other_object_init_method_intptr_intptr in mono_aot_0nLeOF.o wrapper_other_object_init_method_gshared_mrgctx_intptr_intptr_intptr in mono_aot_0nLeOF.o wrapper_other_object_init_method_gshared_this_intptr_intptr_intptr in mono_aot_0nLeOF.o wrapper_other_object_init_method_gshared_vtable_intptr_intptr_intptr in mono_aot_0nLeOF.o

I think it comes from method prolog trying to invoke the init wrapper, but I would expect invocation of wrapper_other_object_init_method_intptr_intptr instead of _mono_aot_HelloWorld_init_method. @vargaz What we are doing wrong?

FWIW, I am not 100% how this should work but I think the body of _mono_aot_HelloWorld_init_method is never generated, looking at the compiler output: https://gist.github.com/ivanpovazan/96542ae24088a4a5e735d414b92e4c9e
there are only jumps to that symbol bl _mono_aot_HelloWorld_init_method, but it is not generated anywhere.

Signed-off-by: Vlad <vladionescu1399@gmail.com>

LeVladIonescu · 2023-10-31T00:37:09Z

In the current state, the problem with the symbol of the init wrapper persists. There is a mismatch between what is emitted in the non llvm case and what symbol is the init wrapper trying to call.

ghost · 2023-11-30T05:01:21Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

Vlad - Alexandru Ionescu and others added 6 commits June 30, 2023 12:27

Preliminary work for nollvm init method

a6b88f9

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MBP-5.guest.corp.microsoft.com>

Added cee opcodes for amodule and init_bitset

c6fc40a

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Passing only method_index to init wrapper

95a8470

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Small changes and cleanup

03e1389

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Commented

9140661

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Conflicts

aad02cc

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

LeVladIonescu added NO-REVIEW Experimental/testing PR, do NOT review it area-Codegen-AOT-mono labels Jul 18, 2023

LeVladIonescu self-assigned this Jul 18, 2023

LeVladIonescu requested review from vargaz, lambdageek, SamMonoRT, thaystg and marek-safar as code owners July 18, 2023 10:30

Enabled for arm64 for CI purposes

9976eaf

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

LeVladIonescu changed the title ~~[DRAFT][mono][aot] Implementation of nollvm init method~~ [mono][aot] Implementation of nollvm init method Jul 18, 2023

LeVladIonescu removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jul 18, 2023

LeVladIonescu requested review from kotlarmilos and ivanpovazan July 18, 2023 11:04

Leftover

49dcf2e

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

LeVladIonescu commented Jul 18, 2023

View reviewed changes

This was referenced Jul 18, 2023

Work item tests crash on OSX x64 Debug #87287

Closed

LibraryImportGenerator.Unit.Tests crashed in CI #87951

Closed

lambdageek reviewed Jul 19, 2023

View reviewed changes

kotlarmilos reviewed Jul 19, 2023

View reviewed changes

Small changes, added a flag for later use

1dbcc28

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Added cmake flag

2bad646

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

lambdageek reviewed Jul 21, 2023

View reviewed changes

Vlad - Alexandru Ionescu and others added 3 commits July 21, 2023 17:20

Cleanup

0e4b036

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>

Update src/mono/mono/mini/mini-arm64.c

00a9b41

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>

Added flag and converting this PR to draft

f5eee3e

Signed-off-by: Vlad <vladionescu1399@gmail.com>

LeVladIonescu changed the title ~~[mono][aot] Implementation of nollvm init method~~ [DRAFT][mono][aot] Implementation of nollvm init method Aug 17, 2023

LeVladIonescu added the NO-REVIEW Experimental/testing PR, do NOT review it label Aug 17, 2023

build-analysis bot mentioned this pull request Aug 18, 2023

Insufficient memory of docker containers on CI dotnet/dnceng#450

Open

3 tasks

kotlarmilos added 2 commits August 18, 2023 14:45

Merge branch 'dotnet:main' into noLLVM

bfe9c5e

Fix AOT compilation to generate init wrapper code

73537b5

kotlarmilos reviewed Aug 18, 2023

View reviewed changes

build-analysis bot mentioned this pull request Aug 18, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

LeVladIonescu marked this pull request as draft September 18, 2023 13:17

Merge branch 'dotnet:main' into noLLVM

cc43423

build-analysis bot mentioned this pull request Oct 18, 2023

Test failure: Wasm.Build.Tests.NativeBuildTests.MonoAOTCross_WorksWithNoTrimming #93522

Closed

Change the symbol for the init wrappers

406e5bd

Signed-off-by: Vlad <vladionescu1399@gmail.com>

build-analysis bot mentioned this pull request Oct 31, 2023

[browser][hybridGlobalization] WBT: Hybrid Globalization fails with AOT #94212

Closed

ghost closed this Nov 30, 2023

github-actions bot locked and limited conversation to collaborators Dec 30, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT][mono][aot] Implementation of nollvm init method #89074

[DRAFT][mono][aot] Implementation of nollvm init method #89074

LeVladIonescu commented Jul 18, 2023

LeVladIonescu commented Jul 18, 2023

LeVladIonescu Jul 18, 2023

lambdageek left a comment

lambdageek Jul 19, 2023

LeVladIonescu Jul 19, 2023

lambdageek Jul 19, 2023

kotlarmilos Jul 19, 2023

LeVladIonescu commented Jul 19, 2023

lambdageek commented Jul 19, 2023

vargaz commented Jul 19, 2023

lambdageek left a comment

lambdageek Jul 21, 2023 •

edited

Loading

lambdageek Jul 21, 2023 •

edited

Loading

LeVladIonescu commented Aug 17, 2023

kotlarmilos Aug 18, 2023

ivanpovazan Aug 18, 2023 •

edited

Loading

LeVladIonescu commented Oct 31, 2023

ghost commented Nov 30, 2023

[DRAFT][mono][aot] Implementation of nollvm init method #89074

[DRAFT][mono][aot] Implementation of nollvm init method #89074

Conversation

LeVladIonescu commented Jul 18, 2023

LeVladIonescu commented Jul 18, 2023

LeVladIonescu Jul 18, 2023

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek Jul 19, 2023

Choose a reason for hiding this comment

LeVladIonescu Jul 19, 2023

Choose a reason for hiding this comment

lambdageek Jul 19, 2023

Choose a reason for hiding this comment

kotlarmilos Jul 19, 2023

Choose a reason for hiding this comment

LeVladIonescu commented Jul 19, 2023

lambdageek commented Jul 19, 2023

vargaz commented Jul 19, 2023

lambdageek left a comment

Choose a reason for hiding this comment

lambdageek Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

lambdageek Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

LeVladIonescu commented Aug 17, 2023

kotlarmilos Aug 18, 2023

Choose a reason for hiding this comment

ivanpovazan Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

LeVladIonescu commented Oct 31, 2023

ghost commented Nov 30, 2023

lambdageek Jul 21, 2023 •

edited

Loading

lambdageek Jul 21, 2023 •

edited

Loading

ivanpovazan Aug 18, 2023 •

edited

Loading