Skip to content

[move-function] Add support for loadable vars #40478

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

Merged

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Dec 9, 2021

I had to make two changes to do this:

  1. In the first commit I made some changes to how we early in the pipeline handle mark_unresolved_move_addr and move_value [allows_diagnostics] (see the commit log for more info).
  2. I built on top of that to add a small change that teaches that early munging how to modify the IR to make it so we properly support loadable vars.

@gottesmm
Copy link
Contributor Author

gottesmm commented Dec 9, 2021

@swift-ci smoke test

… mark_unresolved_move_addr.

To give a bit more information, currently the way the move function is
implemented is that:

1. SILGen emits a builtin "move" that is called within the function _move in the
   stdlib.

2. Mandatory Inlining today if the final inlined type is address only, inlines
   builtin "move" as mark_unresolved_move_addr. Otherwise, if the inlined type
   is loadable, it performs a load [take] + move [diagnostic] + store [init].

3. In the diagnostic pipeline before any mem optimizations have run, we run the
   move checker for addresses. This eliminates /all/ mark_unresolved_move_addr
   as part of emitting diagnostics. In order to make this work, we perform a
   small optimization before the checker runs that moves the
   mark_unresolved_move_addr from being on temporary alloc_stacks to the true
   base underlying address we are trying to move. This optimization is necessary
   since _move is generic and often times SILGen will emit this temporary that
   we do not want.

4. Then after we have run the guaranteed mem optimizations, we run the object
   based move checker emitting diagnostics.

This PR changes the scheme above to the following:

1. SILGen emits a builtin "move" that is called within the function _move in the
   stdlib.

2. Mandatory Inlining inlines builtin "move" as mark_unresolved_move_addr.

3. In the diagnostic pipeline before we have run any mem optimizations and
   before we have run the actual move address checker, we massage the IR as we
   do above but in a separate pass where in addition we try to match this pattern:

     ```
     %temporary = alloc_stack $LoadableType
     store %1 to [init] %temporary : $*LoadableType
     mark_unresolved_move_addr %temporary to %otherAddr : $*LoadableType
     destroy_addr %temporary : $*LoadableType
     ```

   and transform it to:

     ```
     %temporary = alloc_stack $LoadableType
     %2 = move_value [allows_diagnostics] %1 : $*LoadableType
     store %2 to [init] %temporary : $*LoadableType
     destroy_addr %temporary : $*LoadableType
     ```

   ensuring that the object move checker will handle this.

4. Then after we have run the guaranteed mem optimizations, we run the object
   based move checker emitting diagnostics.
This is done by extending the support in the previous PR for

   a. If we see the following pattern:

     ```
     %0 = alloc_stack [lexical] $LoadableType, var
     ... *init of %0* ...
     %1 = load [copy] %0 : $*LoadableType
     %temporary = alloc_stack $LoadableType
     store %1 to [init] %temporary : $*LoadableType
     mark_unresolved_move_addr %temporary to %otherAddr : $*LoadableType
     destroy_addr %temporary : $*LoadableType
     ```

   we transform it to:

     ```
     %0 = alloc_stack [lexical] $LoadableType, var
     ... *init of %0* ...
     %1 = load [copy] %0 : $*LoadableType
     %temporary = alloc_stack $LoadableType
     store %2 to [init] %temporary : $*LoadableType
     mark_unresolved_move_addr %0 to %otherAddr : $*LoadableType
     destroy_addr %temporary : $*LoadableType
     ```

   For safety reasons, we always make sure that %0 isn't destroyed in between
   the load [copy] and the mark_unresolved_move_addr. After this runs, we can
   use the address verifier on these types of vars with some additional work
   that I am doing in a subsequent commit.
…ical borrows.

A lexical borrow is creating a new entity that should be checked separately from
the operand of the lexical borrow.
This is safe to do since:

1. _move is already an always emit into client transparent function, so no ABI
   impact comes from it.

2. _move is underscored so it is a non-stable stdlib API meaning we aren't
   providing any guarantees here.

3. Code that doesn't use _move will not be effected by this so there isn't an
   impact on other code.
@gottesmm gottesmm force-pushed the pr-cfbd8a7087ef5ca3ec578da8993a686ad7a50077 branch from ee73b88 to deb4541 Compare December 9, 2021 20:51
@gottesmm
Copy link
Contributor Author

gottesmm commented Dec 9, 2021

@swift-ci smoke test

@gottesmm gottesmm merged commit aeceaf7 into swiftlang:main Dec 10, 2021
@gottesmm gottesmm deleted the pr-cfbd8a7087ef5ca3ec578da8993a686ad7a50077 branch December 10, 2021 00:19
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 this pull request may close these issues.

1 participant