[mono][interp] Ensure call offset is greater than offset of other active calls #80728

kotlarmilos · 2023-01-17T10:08:50Z

This PR fixes #80175 by introducing deferred calls resolution. The algorithm for allocating offsets for the arguments of a call must ensure that the base offset of the call must be greater than the offset of any argument of other active call args.

For each call, function end_active_call computes the call offset of each call argument starting from a base offset. The base offset is computed as max offset of all call offsets on which the call depends. Deferred call stack ensures that all call offsets on which the call depends are calculated before the call resolution.

… call must be greater than the offset of any argument of other active call args. Function allocates an offset of resolved calls following the constraint.

…call instruction. Use glist data structure allocated from the td->mempool.

ghost · 2023-01-17T10:09:09Z

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

Issue Details

This PR fixes #80175 by introducing deferred calls resolution. The algorithm for allocating offsets for the arguments of a call must ensure that the base offset of the call must be greater than the offset of any argument of other active call args.

For each call, function end_active_call computes the call offset of each call argument starting from a base offset. The base offset is computed as max offset of all call offsets on which the call depends. Deferred call stack ensures that all call offsets on which the call depends are calculated before the call resolution.

Author:	kotlarmilos
Assignees:	kotlarmilos
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

BrzVlad · 2023-01-17T12:57:16Z

src/mono/mono/mini/interp/transform.c

 	td->ip += 5;
-	td->last_ins->info.call_args = call_args;
+	init_last_ins_call (td);
+	td->last_ins->info.call_info->call_args = call_args;


The call_args field was incorrectly set here. We should only init in places where INTERP_INST_FLAG_CALL is set, aka above

In interp_transform_call there is https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/interp/transform.c#L3556-L3577 that doesn't set INTERP_INST_FLAG_CALL which is probably correct. I will add there allocation of InterpCallInfo structure if it doesn't exist.

BrzVlad · 2023-01-17T12:58:30Z

src/mono/mono/mini/interp/transform.h

-		int *call_args;
+		InterpCallInfo *call_info;
 	} info;
+	InterpCallInfo *call_info;


duplicate addition

BrzVlad · 2023-01-17T13:07:53Z

src/mono/mono/mini/interp/transform.c

+	call->call_info->call_deps = NULL;
+	for (int i = 0; i < ac->active_calls_count; i++)
+		call->call_info->call_deps = g_slist_prepend_mempool (td->mempool, call->call_info->call_deps, ac->active_calls [i]);
+	ac->deferred_calls = g_slist_prepend_mempool (td->mempool, ac->deferred_calls, call);


We seem to be allocating a list entry for the most common case of no active calls / resolve arg offsets immediately.

BrzVlad · 2023-01-17T13:09:07Z

src/mono/mono/mini/interp/transform.c

-		call_args [0] = new_var;
-		call_args [1] = -1;
+	// Push active call that should be resolved onto the stack
+	call->call_info = (InterpCallInfo*)mono_mempool_alloc (td->mempool, sizeof (InterpCallInfo));


wasn't call info already allocated when generating code ?

You are right. It was an issue due to duplication of InterpCallInfo in InterpInst.

BrzVlad · 2023-01-17T13:13:33Z

Once all issues are fixed I would be interested in some quick numbers for compilation time change, just in case. You could run System.Runtime.Tests with tiering disabled, and use MONO_TIME_TRACK on interp_alloc_offsets, with and without your change.

… in interp_transform_call.

…alls.

BrzVlad · 2023-01-18T15:41:47Z

src/mono/mono/mini/interp/transform.c

 #endif
 		}
-		td->last_ins->flags |= INTERP_INST_FLAG_CALL;
+		init_last_ins_call (td);


we should set call_args here and not allocate the call info below, for non-call instructions

Now, there is a check if (td->last_ins->flags & INTERP_INST_FLAG_CALL) { so I haven't set call_args here. Let me know if it should be updated.

BrzVlad · 2023-01-18T16:00:27Z

src/mono/mono/mini/interp/transform.c

-	ac->active_calls [ac->active_calls_count].call = call;
-	ac->active_calls [ac->active_calls_count].param_size = get_call_param_size (td, call);
-	ac->param_size += ac->active_calls [ac->active_calls_count].param_size;
+	ac->active_calls [ac->active_calls_count] = (InterpInst*)mono_mempool_alloc (td->mempool, sizeof (InterpInst));


this code seems accidental

kotlarmilos · 2023-01-19T12:06:27Z

@BrzVlad good catch in #80795, it fixed the failing tests.

kotlarmilos · 2023-01-19T17:06:24Z

Once all issues are fixed I would be interested in some quick numbers for compilation time change, just in case. You could run System.Runtime.Tests with tiering disabled, and use MONO_TIME_TRACK on interp_alloc_offsets, with and without your change.

Compilation time of interp_alloc_offsets in μs for System.Runtime.Tests with tiering disabled.

Run	Incorrect offset allocation fix branch	Main branch
1	90438	89927
2	90884	89905
3	92977	97611
4	90264	92051
5	94591	91173
6	90077	89698
7	89900	91660
8	89433	89643
9	90259	93393
10	94498	89805
--	--	--
AVG	91332,10	91486,60
STDEV	1940,06	2493,31

BrzVlad · 2023-01-20T14:21:13Z

src/mono/mono/mini/interp/transform.c

+		InterpInst *deferred_call = call;
+		while (deferred_call) {
+			// `base_offset` is a relative offset (to the start of the call args stack) where the args for this call reside.
+			int base_offset = 0;


I think a comment here explaining why call_offset is guaranteed to be set for all deps wouldn't hurt. Something like: The deps for a call represent the list of active calls at the moment when the call ends. This means that all deps for a call end after the call in question. Given we iterate over the list of deferred calls from the last to the first one to end, all deps of a call are guaranteed to have been processed at this point.

BrzVlad · 2023-01-20T14:23:05Z

src/mono/mono/mini/interp/transform.c

+ * Function first removes the call from an array of active calls. If a match is found,
+ * the call is removed from the array by moving the last entry into its place. Otherwise, it is a call without arguments.
+ *
+ * Call is push onto the stack with an array of other active calls that need to be resolved first.


I think this comment is outdated. You might want to double check the explanation for this method.

BrzVlad · 2023-01-20T14:23:25Z

src/mono/mono/mini/interp/transform.c

 }

+static void
+init_last_ins_call (TransformData *td) {


newline here

BrzVlad · 2023-01-20T14:53:45Z

src/mono/mono/mini/interp/transform.c

+ * Function first removes the call from an array of active calls. If a match is found,
+ * the call is removed from the array by moving the last entry into its place. Otherwise, it is a call without arguments.
+ *
+ * If there are active calls, the call is push onto the stack with an array of other active calls on which the call in question depends.


I was referring to

call is push onto the stack with an array of other active calls

Seemed like old comment from your original implementation where we were pushing the current call together with all active calls. Which we no longer do now.

If there are active calls, it is pushed onto the stack as a deferred call. It also contains an array of other active calls. Let me change it to be clear that the array is not pushed onto the stack but exists as a part of InterpInst.

…ive calls (dotnet#80728) * Add deferred call resolution for active calls. The base offset of the call must be greater than the offset of any argument of other active call args. The base offset is computed as max offset of all call offsets on which the call depends. Stack ensures that all call offsets on which the call depends are calculated before the call in question, by deferring calls from the last to the first one. * Move call_args to InterpCallInfo struct. Allocate InterpCallInfo per call instruction.

kotlarmilos added 3 commits January 13, 2023 15:54

Add deferred call resolution for active calls. The base offset of the…

b5982f5

… call must be greater than the offset of any argument of other active call args. Function allocates an offset of resolved calls following the constraint.

Move call_args to InterpCallInfo struct. Allocate InterpCallInfo per …

d4e8177

…call instruction. Use glist data structure allocated from the td->mempool.

Update function comment.

a05b419

kotlarmilos requested a review from BrzVlad as a code owner January 17, 2023 10:08

kotlarmilos self-assigned this Jan 17, 2023

kotlarmilos requested a review from vargaz as a code owner January 17, 2023 10:08

ghost added the area-Codegen-Interpreter-mono label Jan 17, 2023

kotlarmilos added this to the 8.0.0 milestone Jan 17, 2023

BrzVlad reviewed Jan 17, 2023

View reviewed changes

build-analysis bot mentioned this pull request Jan 17, 2023

Build fails with "eng/common/tools.sh: line 474: 537 Segmentation fault" #76759

Closed

This was referenced Jan 17, 2023

Methodical_others test JIT/Methodical/Coverage/copy_prop_byref_to_native_int crashing #69832

Open

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

kotlarmilos added 3 commits January 17, 2023 17:31

Remove duplication of InterpCallInfo. Add allocation of InterCallInfo…

0190422

… in interp_transform_call.

Optimize end_active_call to resolve call immediately when no active c…

3fa4bf4

…alls.

Set call_args to NULL in init_last_ins_call.

d654e5c

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

Tracking issue for CI build timeouts #76454

Closed

Check if call_info struct exists before allocating from mempool

0a5bad2

BrzVlad reviewed Jan 18, 2023

View reviewed changes

kotlarmilos added 4 commits January 18, 2023 17:42

Remove wrong allocation of InterpInst for active calls.

51fc693

Merge branch 'main' into bugfix/incorrect-offset-allocation

806edcf

Fix reference to call_args in interp_transform_call.

2786d79

Merge branch 'dotnet:main' into bugfix/incorrect-offset-allocation

ad52c21

runfoapp bot mentioned this pull request Jan 19, 2023

Infra improvements for Helix #68176

Closed

BrzVlad reviewed Jan 20, 2023

View reviewed changes

Update comments.

7837536

BrzVlad reviewed Jan 20, 2023

View reviewed changes

Update comment.

1031caf

BrzVlad approved these changes Jan 20, 2023

View reviewed changes

Fix typo and extend a comment.

f051205

kotlarmilos merged commit 7ec3634 into dotnet:main Jan 21, 2023

kotlarmilos deleted the bugfix/incorrect-offset-allocation branch January 21, 2023 08:03

ghost locked as resolved and limited conversation to collaborators Feb 20, 2023

[mono][interp] Ensure call offset is greater than offset of other active calls #80728

[mono][interp] Ensure call offset is greater than offset of other active calls #80728

Uh oh!

Conversation

kotlarmilos commented Jan 17, 2023

Uh oh!

ghost commented Jan 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotlarmilos Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrzVlad commented Jan 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotlarmilos commented Jan 19, 2023

Uh oh!

kotlarmilos commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kotlarmilos Jan 17, 2023 •

edited

Loading

kotlarmilos commented Jan 19, 2023 •

edited

Loading