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

Remove pointers from the Sway IR. #3663

Merged
merged 2 commits into from
Jan 2, 2023
Merged

Remove pointers from the Sway IR. #3663

merged 2 commits into from
Jan 2, 2023

Conversation

otrho
Copy link
Contributor

@otrho otrho commented Dec 31, 2022

Argh, this is hard

This is the result of several attempts (made over several weeks) to attack #2819 and is only the very first stage.

  • Initially @vaivaswatha introduced a PR which removed Aggregate and extract_value/insert_value, replacing them with proper pointer types and GEP. This was great, but was too hard to make work with the ASM generation. There are too many assumptions around reference types vs copy types and what is a 'pointer' or not for a change like this to go in yet.
  • I then tried paring it back to only introduce proper pointer types, leaving GEP for later, but still making it work with ASM gen (especially contract calls and storage operations) was too hard.

The problem

get_ptr was a bit nasty, as we use it to get the address of locals, but also to cast them to different types and also to index, a bit like GEP will.

Also, using Pointer for ref mut function args was a mistake as they were designed only for local variables.

Solution

This PR does as little as possible. The baby steps approach is absolutely required for this task and this is the first one.

To remove any ambiguity around how pointers should work this PR removes them altogether from IR, as they were actually not strictly doing anything. In particular it removes Pointer from Type and get_ptr is now get_local which is actually what 'pointers' were.

We will re-introduce pointers as actual types in an upcoming PR.

So now we have get_local which does nothing more than get a local variable address. Casting and indexing has been shunted to a new cast_ptr instruction as a stopgap. When we introduce GEP we can remove cast_ptr, or at least formalise it into a better/general cast.

This PR also uses the newer quad storage operations which support multiple slots, so the get_ptr -style indexing isn't actually required AFAICT.

Future stages

  • Next we can convert Type into an arena and remove Aggregate. This is another ambiguity around 'reference types' which needs to be removed. The IR should not have any concept of copy types or reference types. This is a Sway thing and isn't portable to different back-ends.
  • Then we can introduce Pointer as an actual Type. This will let us be explicit and strict about memory accesses and genuinely remove the notion of 'reference types'. We should consider opaque pointers as an alternative to pointer casting, which is required for dealing with storage slots and arrays of b256s.
  • Then we replace extract_value and insert_value with GEP.
  • Also work out how casting will work.

@otrho otrho force-pushed the otrho/remove-ir-ptr branch 3 times, most recently from 5efbcb1 to 90b5e11 Compare January 1, 2023 23:22
@otrho otrho marked this pull request as ready for review January 1, 2023 23:54
@otrho otrho requested review from a team, mohammadfawaz and vaivaswatha January 1, 2023 23:54
@otrho otrho self-assigned this Jan 1, 2023
@otrho otrho added code quality compiler: ir IRgen and sway-ir including optimization passes compiler: codegen Everything to do with IR->ASM, register allocation, etc. labels Jan 1, 2023
@otrho otrho enabled auto-merge (squash) January 1, 2023 23:56
The pointers in the IR are not really pointers, but are local variables.
We want to add proper pointers to the IR though and naming the local
variables as pointers adds ambiguity.

This change renames `get_ptr` to `get_local` and removes `ptr` and `mut`
from the serialised output.  `get_local` no longer performs casts so
`cast_ptr` has been added as a stopgap until we add a better solution.
Copy link
Contributor

@vaivaswatha vaivaswatha left a comment

Choose a reason for hiding this comment

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

The change looks good to me. I didn't understand the change to fn compile_union_or_string_storage_(read/write) though. Hopefully the 2nd reviewer can check that.

Copy link
Contributor

@mohammadfawaz mohammadfawaz left a comment

Choose a reason for hiding this comment

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

Awesome. The direction of this seems very reasonable to me.

sway-core/src/ir_generation/function.rs Show resolved Hide resolved
sway-core/src/ir_generation/function.rs Show resolved Hide resolved
@otrho otrho merged commit 76f5653 into master Jan 2, 2023
@otrho otrho deleted the otrho/remove-ir-ptr branch January 2, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality compiler: codegen Everything to do with IR->ASM, register allocation, etc. compiler: ir IRgen and sway-ir including optimization passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants