Skip to content

Fix SILCombine of existential applies #78412

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

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Jan 2, 2025

#78354 enabled silcombine to propagate concrete type of existentials in ossa. However when the concrete value is owned it is consumed by the init_existential_ref forwarding operation. In this case we create an unchecked cast of the opened existential to the concrete type and use that in the apply.

@meg-gupta meg-gupta requested a review from eeckstein as a code owner January 2, 2025 21:15
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta meg-gupta marked this pull request as draft January 3, 2025 07:38
@eeckstein
Copy link
Contributor

I don't think this is a good idea. Inserting a copy+destroy might be worse than not propagating the concrete type of existential. The optimizer should never insert copies which are not there in the first place.

I suggest to just not do the optimization in such a case.
The correct solution would be to insert a borrow scope around the lifetime of the concrete value and do the optimization on the borrowed value. But this is more complicated and we can do this later - if we see that it is a common case which matters.

@meg-gupta meg-gupta force-pushed the fixsilcombine branch 2 times, most recently from 183582e to e4c263c Compare January 3, 2025 19:41
This code was previously creating illegal unchecked_ref_cast
 with same source and destination.
@meg-gupta meg-gupta force-pushed the fixsilcombine branch 3 times, most recently from d1f964e to 13027ae Compare January 4, 2025 00:40
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta meg-gupta marked this pull request as ready for review January 7, 2025 01:37
@eeckstein
Copy link
Contributor

This PR fixes some source compatibility failures: rdar://142298318

Don't use previously found owned concrete values in ossa. They will consumed by
forwarding operations like init_existential_ref. Instead create an unconditional cast
of the opened existential to concrete type and use that to create a concrete apply.
@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta
Copy link
Contributor Author

@swift-ci test source compatibility

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.

lgtm

@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test Linux platform

@meg-gupta meg-gupta enabled auto-merge January 7, 2025 22:57
@meg-gupta
Copy link
Contributor Author

@swift-ci smoke test Linux platform

@meg-gupta meg-gupta merged commit 0634bab into swiftlang:main Jan 8, 2025
5 of 7 checks passed
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