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][interp] Remove dependence on svar/dvar when computing call offset #81017

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Jan 23, 2023

On optimized code, we made sure we always had a svar set, even if the call had no args. We then resolved the offset of the call as the offset of the first arg.
On unoptimized code, we made sure we always had a dvar set, even if the call has void return. We then resolved the offset of the call as the offset of the dvar. As we are introducing alignment guarantees of param area and, in the future, the dvar (only if it is simd type), the offset of the dvar and svar can end up with quite different offsets.

This commit solves these complications by introducing a call_offset field in the call info data, which represents the offset of the call args. This field is set either by the var offset allocator in optimized case or on the fly, during compilation, in unoptimized case. The dvar offset is allocated independently. When emitting compacted instruction for calls, we do it now in an unified way between optimized and unoptimized compilation, by offsetting call_offset from td->param_area_offset, which is now initialized in both modes.

On optimized code, we made sure we always had a svar set, even if the call had no args. We then resolved the offset of the call as the offset of the first arg.
On unoptimized code, we made sure we always had a dvar set, even if the call has void return. We then resolved the offset of the call as the offset of the dvar. As we are introducing alignment guarantees of param area and, in the future, the dvar (only if it is simd type), the offset of the dvar and svar can end up with quite different offsets.

This commit solves these complications by introducing a call_offset field in the call info data, which represents the offset of the call args. This field is set either by the var offset allocator or on the fly, during compilation, in unoptimized case. The dvar offset is allocated independently and when emitting compacted instruction for calls, we do it now in an unified way between optimized and unoptimized compilation.
@ghost
Copy link

ghost commented Jan 23, 2023

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

Issue Details

On optimized code, we made sure we always had a svar set, even if the call had no args. We then resolved the offset of the call as the offset of the first arg.
On unoptimized code, we made sure we always had a dvar set, even if the call has void return. We then resolved the offset of the call as the offset of the dvar. As we are introducing alignment guarantees of param area and, in the future, the dvar (only if it is simd type), the offset of the dvar and svar can end up with quite different offsets.

This commit solves these complications by introducing a call_offset field in the call info data, which represents the offset of the call args. This field is set either by the var offset allocator in optimized case or on the fly, during compilation, in unoptimized case. The dvar offset is allocated independently. When emitting compacted instruction for calls, we do it now in an unified way between optimized and unoptimized compilation, by offsetting call_offset from td->param_area_offset, which is now initialized in both modes.

Author: BrzVlad
Assignees: BrzVlad
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 23, 2023

/azp run runtime-wasm

@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 23, 2023

/azp run runtime-wasm-libtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

Overall looks good. I was thinking if there is a way to encapsulate initialization of call_info (particularly call_args and call_offset) into a helper function as there are many cases.

src/mono/mono/mini/interp/transform.c Show resolved Hide resolved
src/mono/mono/mini/interp/transform.c Show resolved Hide resolved
src/mono/mono/mini/interp/transform.c Show resolved Hide resolved
src/mono/mono/mini/interp/transform.c Show resolved Hide resolved
@@ -147,11 +147,12 @@ struct _InterpCallInfo {
// in the order they are pushed to the stack. This makes it easy to find
// all source vars for these types of opcodes. This is terminated with -1.
int *call_args;
int call_offset;
Copy link
Member

Choose a reason for hiding this comment

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

call_end_offset has become temporary variable needed only during the offset allocation and it might be removed from the call_info struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

We added the call_end_offset field so we can easily map between call InterpInst to allocated offset, without use of other data structures and lookups. Did you have an alternative for this ?

Copy link
Member

Choose a reason for hiding this comment

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

Without other data structures, no. It can be inferred from base_offset + call_args but it is probably inefficient.

@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 24, 2023

Overall looks good. I was thinking if there is a way to encapsulate initialization of call_info (particularly call_args and call_offset) into a helper function as there are many cases.

I would agree. There is certain room for refactoring in general, we can think about it in the future.

@BrzVlad BrzVlad merged commit 98629a1 into dotnet:main Jan 24, 2023
BrzVlad added a commit to BrzVlad/runtime that referenced this pull request Feb 6, 2023
Assumption that return offset is identical to location of param offset for this opcode is no longer true. Set the param_offset explicitly, separate from the return, similar to dotnet#81017
BrzVlad added a commit that referenced this pull request Feb 6, 2023
* [mono][interp] Remove unused method

* [mono][interp] Optimize code just in case

* [mono][interp] Align simd types to 16 bytes by default

All interp vars (args, il locals and other allocated vars) are now aligned to 16 byte offsets.

* [mono][interp] Add svar arg to MINT_NEWOBJ_SLOW_UNOPT

Assumption that return offset is identical to location of param offset for this opcode is no longer true. Set the param_offset explicitly, separate from the return, similar to #81017

* [mono][interp] Disable assertion on hot path

* [mono][interp] Remove some duplicate code
@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2023
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.

2 participants