Skip to content

[OnoneSimplification] Fixed isDestroyed(after:) #70552

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

Closed
wants to merge 1 commit into from

Conversation

nate-chandler
Copy link
Contributor

@nate-chandler nate-chandler commented Dec 20, 2023

The previous implementation just checked that a value's only uses besides the begin_borrow were destroys. That's insufficient to say the value is destroyed after the borrow (i.e. that all its destroys are dominated by the borrow). Add the relevant dominance check.

It would be better not to compute the dominance tree here, but that compile-time issue can be fixed separately.

rdar://119873930

The previous implementation just checked that a value's only uses
besides the begin_borrow were destroys.  That's insufficient to say the
value is destroyed _after_ the borrow (i.e. that all its destroys are
dominated by the borrow).  Add the relevant dominance check.

It would be better not to commpute the dominance tree here, so that
performance issue can be fixed separately.

rdar://119873930
@nate-chandler nate-chandler requested a review from atrick December 20, 2023 02:04
@nate-chandler
Copy link
Contributor Author

@swift-ci please 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.

Great thanks.

This is the complete/straightforward fix. Later we may want to remove dominance from simplification. But that's a separate discussion. Ideally, simplifications would never invalidate dominance, so it should be ok to do this in a perfect world.

@nate-chandler nate-chandler marked this pull request as ready for review December 20, 2023 02:58
@nate-chandler
Copy link
Contributor Author

@swift-ci please build toolchain

Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

Sorry, we cannot do this.
Simplifications can modify the CFG and therefore the domtree. We have several of them. As simplifications run inside a single pass without analysis invalidation after each simplification, they must not use any analysis. It would lead to really nasty bugs.

@eeckstein
Copy link
Contributor

Here is another PR which fixes the problem: #70559

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