Skip to content

[pmo] Fix load_borrow for strong control equivalence issues. #28208

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

This entailed untangling a few issues. I have purposefully only fixed this for
now for load_borrow to make the change in tree smaller (the reason for my
splitting the code paths in a previous group of commits). I am going to follow
up with a load copy version of the patch. I think that the code should be the
same. The interesting part about this is that all of these code conditions are
caught by the ownership verifier, suggesting to me that the code in the
stdlib/overlays is simple enough that we didn't hit these code patterns.

Anyways, the main change here is that we now eliminate control equivalence
issues arising from @owned values that are not control equivalent being
forwarded into aggregates and phis. This would cause our outer value to be
destroyed early since we would be destroying it once for every time iteration in
a loop.

The way that this is done is that (noting that this is only for borrows today):

  • The AvailableValueAggregator always copies values at available value points to
    lifetime extend the values to the load block. In the load block, the
    aggregator will take the incoming values and borrow the value to form tuples,
    structs as needed. This new guaranteed aggregate is then copied. If we do not
    need to form an aggregate, we just copy. Since the copy is in our load block,
    we know that we can use it for our load_borrow. Importantly note how we no
    longer allow for these aggregates to forward owned ownership preventing the
    control equivalence problem above.

  • Since the aggregator may use the SSA updater if it finds multiple available
    values, we need to also make sure that any phis are strongly control
    equivalent on all of its incoming values. So we insert copies along those
    edges and then lifetime extend the phi as appropriate.

  • Since in all of the above all copy_value inserted by the available value
    aggregator are never consumed, we can just add destroy_values in the
    appropriate places and everything works.

This entailed untangling a few issues. I have purposefully only fixed this for
now for load_borrow to make the change in tree smaller (the reason for my
splitting the code paths in a previous group of commits). I am going to follow
up with a load copy version of the patch. I think that the code should be the
same. The interesting part about this is that all of these code conditions are
caught by the ownership verifier, suggesting to me that the code in the
stdlib/overlays is simple enough that we didn't hit these code patterns.

Anyways, the main change here is that we now eliminate control equivalence
issues arising from @owned values that are not control equivalent being
forwarded into aggregates and phis. This would cause our outer value to be
destroyed early since we would be destroying it once for every time iteration in
a loop.

The way that this is done is that (noting that this is only for borrows today):

* The AvailableValueAggregator always copies values at available value points to
  lifetime extend the values to the load block. In the load block, the
  aggregator will take the incoming values and borrow the value to form tuples,
  structs as needed. This new guaranteed aggregate is then copied. If we do not
  need to form an aggregate, we just copy. Since the copy is in our load block,
  we know that we can use it for our load_borrow. Importantly note how we no
  longer allow for these aggregates to forward owned ownership preventing the
  control equivalence problem above.

* Since the aggregator may use the SSA updater if it finds multiple available
  values, we need to also make sure that any phis are strongly control
  equivalent on all of its incoming values. So we insert copies along those
  edges and then lifetime extend the phi as appropriate.

* Since in all of the above all copy_value inserted by the available value
  aggregator are never consumed, we can just add destroy_values in the
  appropriate places and everything works.
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm requested a review from atrick November 12, 2019 07:49
@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - eab9be8

@gottesmm
Copy link
Contributor Author

Failure was in the gyb stuff...

@gottesmm
Copy link
Contributor Author

All of the full tests stuff pass/ran. We only failed late due to a different swiftpm issue.

@gottesmm
Copy link
Contributor Author

erm swiftsyntax diff issue.

@gottesmm
Copy link
Contributor Author

@swift-ci smoke test macOS platform

@gottesmm gottesmm merged commit 1ecc281 into swiftlang:master Nov 12, 2019
@gottesmm gottesmm deleted the pr-7fb1b4c160135b4915ee8f8b8d3b7a1e71ce604d branch November 12, 2019 15:09
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.

2 participants