Skip to content

[fix] Eager specialization shouldn't optimize functions with ownership (yet) #30279

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 1 commit into from
Mar 9, 2020

Conversation

zoecarver
Copy link
Contributor

This patch updates EagerSpecializer to skip functions with ownership. The next step for this pass supporting ownership is updating GenericFuncSpecializer.

@zoecarver zoecarver changed the title Eager specilization shouldn't optimize functions with ownership (yet) [fix] Eager specialization shouldn't optimize functions with ownership (yet) Mar 7, 2020
@zoecarver zoecarver requested a review from gottesmm March 9, 2020 18:09
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.

LGTM. I suppose we're relying on unit tests to tell us if we forget to migrate a pass like this that silently bails out.

@zoecarver
Copy link
Contributor Author

I discovered the issue when removing/adding ownership elimination passes to the pipeline. I'm not sure we have a standard way of catching passes like this.

Could you trigger the CI? I haven't been granted access yet (@shahmishal friendly ping).

@atrick
Copy link
Contributor

atrick commented Mar 9, 2020

@swift-ci smoke test and merge

@gottesmm
Copy link
Contributor

gottesmm commented Mar 9, 2020

@zoecarver I think that there is a check in the utilities that prevents this. That being said, I am actually going to enable this soon on OSSA. I am fine with this going in for now.

@zoecarver
Copy link
Contributor Author

Yes, there's an assertion but, I think it's better to bail rather than fail in this case.

@swift-ci swift-ci merged commit be61002 into swiftlang:master Mar 9, 2020
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.

4 participants