Skip to content

[5.6] Upstream defer handling for the move function #40783

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

Conversation

gottesmm
Copy link
Contributor

Just upstreaming the rest of the commits to 5.6 needed to add move support for reinitializing self in a defer and other fixes that I did on main as well.

@gottesmm gottesmm requested a review from atrick January 10, 2022 18:37
@gottesmm
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - b7a28f2c0c30ddcbdfc7ee3e940ccb24475dfacf

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - b7a28f2c0c30ddcbdfc7ee3e940ccb24475dfacf

@gottesmm
Copy link
Contributor Author

Looks like the SILFunction constructor is slightly different on 5.6 from main. I am going to clean up the history here a little bit/fix that.

…r function.

I want to over time expand this and improve QoI by adding better error messages
by using pattern matching here. So it makes sense just to take care of the
refactoring now before making further changes.

(cherry picked from commit e818b25)
Just a thinko on my part. I think I refactored a copy of this code in the
address checker into getDebugVarName() and then forgot to eliminate the object
from the move value checker...

(cherry picked from commit 4247cfe)
…or to be a local variable instead of a field.

NFC.

(cherry picked from commit 40c6f7d)
…esObjectChecker and into the users of said checker.

This ensures that MoveKillsCopyableAddressesObjectChecker is only ever
processing a single address rather than maintaining this worklist. This will
enable me to reuse parts of it in a simpler way for defer checking.

(cherry picked from commit 5b1ec61)
…ile before more re-organization.

(cherry picked from commit e7c9320)
Just discovered this as I working on other code.

(cherry picked from commit 2ce233b)
…lready added to the address test suites.

(cherry picked from commit 35dbb3e)
…I need to store them.

Specifically, I need to perform the initial gathering of uses for all addresses
at the same time so I can handle their closures all at the same time.

(cherry picked from commit e9be61d)
…t_aliasable defer parameters to out parameters after move analysis.

(cherry picked from commit 5dc8b38)
Returns true if this SILFunction must be a defer.

NOTE: This may return false for defer statements that have been
deserialized without a DeclContext. This means that this is guaranteed to
be correct for SILFunctions in Raw SIL that were not deserialized as
canonical. Thus one can use it for diagnostics.

(cherry picked from commit fe6e3f9)
This dataflow is used to determine if a closure/defer use of a value as an
inout_aliasable value is consume inclusive-or liveness requiring use in a
caller. This information is used to determine if a closure invocation or defer
at the source level acts as a use after move in the source level caller of a
moved var or if the moved var is reinited in the function.

This is done by:

1. Classifying all uses of the inout_aliasable argument as either init, consume,
   use.

2. We then perform two separate dataflows that summarize the effect of the
   closure upon the specific address argument:

   a. First for each individual use, we walk upwards along predecessors to the
      beginning of the function stopping at consumes, inits, and other uses. If
      our dataflow eventually reaches the entrance block, then we know that we
      have a use that should be an error. If all uses are blocked by a consume,
      init, or other use then we know that if our operand was moved in a caller,
      the address is not used after the move at least in this callee.

   b. With that in hand, we then perform a similar dataflow for each individual
      consume operation except that we only stop at init blocks and other
      consume blocks. This leaves us with a set of consuming operations of the
      address that can be considered to be consume up out of the callee. We then
      make sure that these consumes form a post dominating set of upon the
      address or bail. If we bail, we do not handle the move in the caller
      causing us to emit an unhandled move error later after all diagnostics
      have run.

After performing both of these dataflows, our caller function can use our
outputs to emit an error if we had a use and a move is live in the caller or
create a new callee where our address argument is converted from an
inout_aliasable parameter to an out parameter.

(cherry picked from commit d825f2c)
This converts a set of closure inout_aliasable arguments to be out arguments and
fixes up the initial post dominating consume set of these arguments since we are
hoisting the destroy out of the cloned closure.

(cherry picked from commit efd0ff8)
…o that we can test end<->end.

Since this is the final in the series of commits, I also added a bunch of tests
since now we have an end to end implementation that /can/ actually be tested.

(cherry picked from commit 7c7b9ee)
…ssonly) x (let, var)

(cherry picked from commit 0696e31)
(cherry picked from commit 1dc4b47)
These operands are not actually consumed. They are instead vars that are
represented as inout_aliasable closure arguments (or defer arguments) that we
convert to @out parameters after performing move checking.

(cherry picked from commit 8aa25d2)
(cherry picked from commit 68627db)
We can always derive isUpwardsUse, isUpwardsConsume from the result downward
scan result within the struct.

(cherry picked from commit 2669541)
…flow and not in the middle of the closure capture analysis.

Just makes jumping around the file less consuing since the topic is consistent
within the closure captured variable analysis.

(cherry picked from commit 4ae07e6)
…lti-block callers.

I just forgot to include this in the previous PR. I added some more tests around
it.

(cherry picked from commit a734a24)
…tain destroy_addr to insert compensating destroy_addr.

This doesn't work well if one can potentially not have any input destroy_addr...
I actually already know the address without inferring it, so I changed the code
to just use that.

I also added more tests around defer/without defer that exercise this behavior.

(cherry picked from commit b5eca9f)
…ure that we properly error on them.

(cherry picked from commit e6a9ac3)
…error on these.

To improve the diagnostics though I think I am going to have to add a closure
operand analysis to copyable values like I did for copyable addresses...

(cherry picked from commit cfdfff3)
…var via its fields instead of all at once.

In the future we may be able to handle this case, but for now lets just error so
that users are not confused.

(cherry picked from commit 7059fea)
@gottesmm gottesmm force-pushed the release/5.6/move-function-defer-upstreaming branch from b7a28f2 to bc6abd2 Compare January 11, 2022 19:30
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

Forgot to fix the difference in between 5.6 and main... oops. Putting up the patch and restarting testing!

@gottesmm
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

These changes from the previous algorithm all look positive. I'll review the algorithm as a whole separately.

@gottesmm gottesmm merged commit 9f779f0 into swiftlang:release/5.6 Jan 12, 2022
@gottesmm gottesmm deleted the release/5.6/move-function-defer-upstreaming branch January 12, 2022 02:48
gottesmm added a commit to gottesmm/swift that referenced this pull request Jan 13, 2022
…move-function-defer-upstreaming"

This reverts commit 9f779f0, reversing
changes made to ba3aa14.

We don't actually need this in 5.6. I am also in a forthcoming commit, going to
turn off _move since we don't need it in 5.6.
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.

3 participants