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

Memory Layout #5227

Closed
xunilrj opened this issue Oct 25, 2023 · 1 comment · Fixed by #5255
Closed

Memory Layout #5227

xunilrj opened this issue Oct 25, 2023 · 1 comment · Fixed by #5255
Assignees

Comments

@xunilrj
Copy link
Contributor

xunilrj commented Oct 25, 2023

Whilst finishing up #4929 I realized that the memory layout code is scattered through out the code. This poses the obvious problem of having to keep everything in sync, but also will not allow the layout to be controlled using #repr() in the future.

This issue will work as a bookmark to places that needs to be refactored to improve and allow customization:

1 - Data Section: at sway-core/src/asm_generation/fuel/data_section.rs. Special attention will be needed at the from_constant because we don't have a way to describe a enum without hacks. See comments at line 98;

2 - Locals: at sway-core/src/asm_generation/fuel/functions.rs inside alloc_locals and init_locals we also need to take into consideration the memory layout of objects.

3 - Realizing loads: at sway-core/src/asm_lang/allocated_ops.rs inside realize_load.

4 - Size* Intrinsics: Intrinsics that return the size of types and instances also need to take memory layout into consideration.

5 - Calling conventions: We need to also consider all of our calling conventions:
5.1 - Entry call and return: We have a special case when the argument/return value fits into the VM register or not. Depending on the case the layout of basic data types change;
5.2 - Contract calls at sway-core/src/ir_generation/function.rs in compile_contract_call : Similiar to above;

6 - Storage: at sway-core/src/ir_generation/storage.rs

7 - Sroa at sway-ir/src/optimize/sroa.rs needs to know the offset of each field and end up calling get_indexed_type at sway-ir/src/irtype.rs . Of course that in this same file size_in_bytes also needs to take layout into account.

A difficult question is if this will need to affect anything inside our IR. Maybe GEP will need to have access to type information.

@xunilrj xunilrj self-assigned this Oct 25, 2023
@ironcev
Copy link
Member

ironcev commented Oct 27, 2023

Excellent compilation! A clear call to a good refactoring.

@ironcev ironcev self-assigned this Nov 2, 2023
IGI-111 pushed a commit that referenced this issue Nov 24, 2023
## Description

This PR closes #5227 by refactoring access to memory layout information.
The focus of the refactoring was removing the redundant code from the
places which will be affected when references are introduced. Previously
scattered in various places, knowledge about the memory layout of the
types is now provided via (mostly) IR `Type` and (a tiny bit) IR
`Constant`. Thus, the IR layer becomes the single source of truth for
the memory layout. From this perspective, having other IR components
like, e.g., SROA being aware of the memory layout is fine, because the
memory layout assumptions are not leaking outside of the IR layer.

In addition, the PR fixes two bugs:
- allocation of 8x more space for local string arrays, then necessary
- wrong calculation of an offset of a union variant

The code outside of IR where an assumption on memory layout is still
made is in the Storage API. Refactoring storage access code to remove
memory layout assumptions would be a questionable effort because we
anyhow want to refactor the Storage API. Therefore, a warning
TODO-MEMLAY is left in the Storage API code as well as `assert`s.

The `size_bytes_round_up_to_word_alignment` macro is still used in
exactly four places, where an alignment is needed, but not of a type
size, then rather some other structure in memory like, e.g., locals on
the stack.

The PR also fully document memory layout decisions and asserts them with
unit test.

The consumers of the memory layout information like, e.g., `DataSection`
of course still need to be aware of the meaning of the layout
information and use it according to their own use case. E.g.:
- the `DataSection` places constants according to the expected layout
- `asm_generation` `functions` module places local variables on the
stack
- etc

The potential next steps are supporting more optimal layout of
aggregates in terms of memory usage, or even providing configurable
memory layouts. For this, there is a separate issue #5286.

Closes #5227.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

---------

Co-authored-by: xunilrj <xunilrj@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants