-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[DCE] Tweaked code to end borrows before destroys. #39760
[DCE] Tweaked code to end borrows before destroys. #39760
Conversation
@swift-ci please test |
d46e2bf
to
f40f7d6
Compare
@swift-ci please test |
@swift-ci please clean test windows platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code looks like it could be secretly shrinking the borrow scope, and consequently reordering destroys. Not something I expected this pass to do. It's probably ok but either merits
- a warning in the file-level pass comment that this pass shrinks scopes and reoders destroys
- or a FIXME here to make sure the scope is only hoisted over newly inserted destroys. That would require a SmallPtrSet of inserted destroy_value instructions
bd2ae13
to
8dc6ae6
Compare
If a phi argument is dead and it reborrowing it was dependent on some other value, that other value on which it was dependent may have already itself been deleted. In that case, the destroy_value would have been added just before the terminator of the predecessors of the block which contained the dead phi. So, when deciding where to insert the end_borrow, iterate backwards from the end of the block, skipping the terminator updating the insertion point every time a destroy_value instruction is encountered until we hit an instruction with a different opcode. This ensures that no matter how many destroy_values may have been added just before the terminator, the end_borrow will preceed them. This commit just tweaks the preexisting logic that checked for this condition. Specifically, the previous code didn't handle the case where the block contains only a terminator and a destroy_value.
8dc6ae6
to
4d92cce
Compare
Added a FIXME. It seems like destroys might also be getting reordered depending on the ordering of the phis:
->
|
@swift-ci please smoke test and merge |
@swift-ci please smoke test linux platform |
If a phi argument is dead and reborrowing it was dependent on some other value, that other value on which it was dependent may have already itself been deleted. In that case, the destroy_value would have been added just before the terminator of the predecessors of the block which contained the dead phi. So, when deciding where to insert the end_borrow, iterate backwards from the end of the block, skipping the terminator updating the insertion point every time a destroy_value instruction is encountered until we hit an instruction with a different opcode. This ensures that no matter how many destroy_values may have been added just before the terminator, the end_borrow will preceed them.
This commit just tweaks the preexisting logic that checked for this condition. Specifically, the previous code didn't handle the case where the block contains only a terminator and a
destroy_value
.