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

Aggregates and memory management in the IR is generally broken. #2819

Closed
otrho opened this issue Sep 21, 2022 · 4 comments · Fixed by #4336
Closed

Aggregates and memory management in the IR is generally broken. #2819

otrho opened this issue Sep 21, 2022 · 4 comments · Fixed by #4336
Assignees
Labels
code quality compiler: ir IRgen and sway-ir including optimization passes enhancement New feature or request

Comments

@otrho
Copy link
Contributor

otrho commented Sep 21, 2022

Although the distinction between copy and reference types is more explicit within the compiler now, it wasn't always the case and this is apparent in the IR design.

E.g.,

  • Operations like store and load are used on entire aggregates as a blanket 'write' or 'read'.
  • insert_value and extract_value were attempting to simplify using structs but just ambiguate them.
  • Arrays and structs are treated collectively as 'aggregates' but they actually don't share any utility.
  • Constant 'aggregates' are created out of thin air where necessary and just permanently stored (and leaked) on the stack.
  • The use of pointers is confused and inconsistent. get_ptr was initially only for getting the address of locals but now it's for arguments too and should support globals.

I propose at least the following fixes:

  • The removal of insert_value, extract_value, insert_element and extract_element and instead use get_ptr, load and store.
  • load and store are only for writing copy type values to a pointer.
  • Pointers are opaque, a la LLVM.
  • get_ptr is improved to be more like GEP from LLVM.
  • A global data section is introduced and mutability be strictly specified and adhered to. Reference typed constants may then be constructed there at compile time.
@otrho otrho added enhancement New feature or request code quality compiler: ir IRgen and sway-ir including optimization passes labels Sep 21, 2022
@otrho
Copy link
Contributor Author

otrho commented Sep 21, 2022

Also currently get_ptr is compiled to an address and stored in a register and is then ignored. And the address calculations in ASM gen are extremely complicated and still have the potential to just fall over for large values that might not fit in an immediate and/or addressing is often quite redundant, calc'ing the same offsets repeatedly.

So, the ASM gen around memory needs a lot of work.

@otrho
Copy link
Contributor Author

otrho commented Sep 24, 2022

I'm also finding now that managing enums, which are tagged unions in IR and ASM, to be extremely painful in the current ASM gen. In particular, ensuring the padding around the union to make sure the memory accesses are aligned and correct in a union of types of different sizes is very clumsy and should be made explicit in IR with decent memory management.

The workarounds in sway_core/src/asm_generation/data_section.rs need to be removed.

@otrho otrho self-assigned this Oct 14, 2022
otrho added a commit that referenced this issue Oct 20, 2022
…o be inlined.

If we don't we'll try to use the args as pointers and get an ICE.

Will be fixed by #2819
emilyaherbert pushed a commit that referenced this issue Oct 20, 2022
…o be inlined.

If we don't we'll try to use the args as pointers and get an ICE.

Will be fixed by #2819
@otrho
Copy link
Contributor Author

otrho commented Oct 21, 2022

Also thinking that in IR there's no point in making distinctions between mutable pointers and immutable. That sort of correctness can be guaranteed by the compiler and so all pointers might as well be mutable. If we create code which tries to write to the data section then that's a compiler bug, not an issue with IR.

@vaivaswatha
Copy link
Contributor

Summary of discussion on slack:

  • We'll introduce Alloca instructions. Loads and stores directly use Alloca as their pointer operand. (i.e., the Alloca Value has type "Pointer").
  • Rename get_ptr to get_element_ptr and change its semantics to take a value of "any pointer" and do offset computation.
  • The existing Pointer data structure will go away. Pointers will just be any other Value with a "pointer" type.
  • insert/extract value/element instructions will be removed. Since aggregates are always "in-memory", we don't deal with SSA values of aggregates, so these instructions aren't needed. Accessing elements of aggregates will be through get_element_ptr for offset computation and a load/store on that.
  • We will retain a per-function database of locals, and update it when creating Alloca instructions. This is for convenience (such as printing all locals at the beginning etc).
  • All of this, along with the recent block args and mem2reg changes have made it more difficult to map IR vregs to source variables. So we need a name field for every Value. If that's doable in this PR (not necessarily populating the name field for every Value, but at least introducing it and populating it only for Alloca insts), good. If it isn't doable, add a "name" field to the Alloca instruction temporarily so we know what we're referring to when printing a Load/Store/Alloca.

@vaivaswatha vaivaswatha self-assigned this Oct 26, 2022
vaivaswatha added a commit that referenced this issue Jan 9, 2023
This is the second step towards achieving #2819.

Combining Type and Aggregate into a single data structure
enables offset computations for complex indexing (i.e.,
a nesting of arrays and structs) which will be needed when
we introduce GEPs. The new function `get_indexed_type` does
this computation.

The core of this PR is in `irtype.rs`. Everything else is just
to accommodate that.
vaivaswatha added a commit that referenced this issue Jan 14, 2023
This is the second step towards achieving #2819.

Combining Type and Aggregate into a single data structure enables offset
computations for complex indexing (i.e., a nesting of arrays and
structs) which will be needed when we introduce GEPs. The new function
`get_indexed_type` does this computation.

The core of this PR is in `irtype.rs`. Everything else is just to
accommodate that.

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
vaivaswatha added a commit that referenced this issue Mar 23, 2024
The new encoding can handle functions with pointer arguments.

So this change reverts #4899.

Also see #2819, which first introduced this hack.
vaivaswatha added a commit that referenced this issue Apr 16, 2024
The new encoding can handle functions with pointer arguments.

So this change reverts #4899. That PR also added a test which now passes
without the forced inlining.

Also see #2819, which first introduced this hack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality compiler: ir IRgen and sway-ir including optimization passes enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants