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

Fix the computation of the re-borrow flags for guaranteed phi arguments #77527

Merged
merged 10 commits into from
Nov 13, 2024

Conversation

eeckstein
Copy link
Contributor

@eeckstein eeckstein commented Nov 11, 2024

Re-borrow flags were computed based on the presence of forwarding instructions in the incoming values.
This is not correct in all cases, because optimizations can optimize away forwarding instructions.

Now, compute the re-borrow flags based on the presence of an end_borrow in the transitive uses.
To do this:

  1. Automatically set the re-borrow flags for phi-arguments when creating an end_borrow or setting the operand of an end_borrow. This already catches most of the cases.
  2. For some optimizations it's required to explicitly update the re-borrow flags. This is done in updateAllBorrowArguments and updateBorrowArguments.

Re-borrow flags are only set, but never cleared. If an optimization creates a dead-end block by cutting off the control flow before an end_borrow, the re-borrow flags still have to remain without the possibility to re-calculate them from the (now missing) end_borrows.

Fixes a verifier crash: rdar://139280579

@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@swift-ci please test source compatibility

@eeckstein
Copy link
Contributor Author

@swift-ci benchmark

@eeckstein
Copy link
Contributor Author

@swift-ci test macos

…structions when computing enclosing values

For example:

```
  %1 = begin_borrow %0
  %2 = br bb1(%1, %1)
bb1(%3 : @reborrow @guaranteed, %4: @guaranteed):
  // %4 is a guaranteed forwarding phi without any forwarding instructions between the begin_borrow and the incoming value.
```

Also improve the comments
…ting an end_borrow or setting the operand of an end_borrow

This is done for all transitively incoming phi-arguments of an end_borrow operand.
…AllBorrowArguments` and `updateBorrowArguments`
This API computes the re-borrow flags for guaranteed phis based on the presence of forwarding instructions in the incoming values.
This is not correct in all cases, because optimizations can optimize away forwarding instructions.

Fixes a verifier crash: rdar://139280579
With the new re-borrow flag computation, verification can fail before the re-borrow flags are updated at the end of the pass.
…ructions.

In case the control flow ends in a dead-end block there can be begin-borrow instructions which have no corresponding end-borrow uses.
After duplicating such a block, the re-borrow flags cannot be recomputed correctly for inserted phi arguments.
Therefore just disable duplicating such blocks, e.g. in jump-threading.
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@swift-ci benchmark

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.

Nice!

With the new re-borrow flag computation, verification can fail before the re-borrow flags are updated at the end of the pass.

This sort of thing is unfortunate. A good goal is to be able to verify SIL immediately after every transformation, not just after a pass. Otherwise, we don't know what SIL utilities are valid within the pass. I wonder why your other fixes are not sufficient to keep the reborrow flags in a valid state during LoopRotate. It should be enough to avoid rotating if a begin_borrow has no scope-ending uses and then to automatically update the reborrow flag when creating the new phi.

@eeckstein eeckstein merged commit 4934b79 into swiftlang:main Nov 13, 2024
6 checks passed
@eeckstein eeckstein deleted the fix-reborrow-flag branch November 13, 2024 06:21
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