Skip to content

Fix ExistentialSpecializer: Recursive function specialization #22247

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 5, 2019

Conversation

rajbarik
Copy link
Contributor

This PR fixes a bug in ExistentialSpecializer where a recursive function when specialized crashed the compiler. I have attached a test case with the PR.

@rajbarik
Copy link
Contributor Author

@atrick can you accept this PR?
A performance benchmark is almost done for enabling ES by default (#19820).

@rajbarik rajbarik requested a review from atrick January 30, 2019 23:59
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.

The change seems fine. I want to set better precedent with the style of test though. The biggest difficulty in changing SIL today is knowing how to update all the CHECK lines without understanding the original intention. I think this should be a .sil test and it needs a comment to explain the bailout that it exercises.

Copy link
Contributor Author

@rajbarik rajbarik left a comment

Choose a reason for hiding this comment

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

@atrick I added a sil test case. the primary goal for both swift and sil test cases are that they check for signatures and hence function_ref instructions are fully checked.

Please review when you get a chance.

@rajbarik rajbarik force-pushed the raj-recursion-fix branch 2 times, most recently from 40d6600 to 70ac1b2 Compare February 22, 2019 19:26
@rajbarik
Copy link
Contributor Author

@atrick please run tests. I have addressed all your concerns.

@atrick
Copy link
Contributor

atrick commented Feb 22, 2019

@swift-ci test.

@atrick
Copy link
Contributor

atrick commented Feb 22, 2019

@rajbarik is the "Stable with ES in open source" commit supposed to be in this PR. I don't remember reviewing that.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 70ac1b2fb8aae27dcb9ed96c7a1b11f21461d61e

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 70ac1b2fb8aae27dcb9ed96c7a1b11f21461d61e

@rajbarik
Copy link
Contributor Author

@rajbarik is the "Stable with ES in open source" commit supposed to be in this PR. I don't remember reviewing that.

Some git mess. Let me fix this.

@rajbarik
Copy link
Contributor Author

@atrick fixed.

@rajbarik
Copy link
Contributor Author

@atrick please run tests here again. Thanks.

@atrick
Copy link
Contributor

atrick commented Feb 26, 2019

@swift-ci test.

@rajbarik
Copy link
Contributor Author

@atrick any idea why this is not reporting anything for last 3 days?

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 6d9f532

@rajbarik
Copy link
Contributor Author

rajbarik commented Mar 3, 2019

@atrick please advise. I do not any logs for Swift Test Linux Platform.

@atrick
Copy link
Contributor

atrick commented Mar 3, 2019

@swift-ci test linux platform.

@rajbarik rajbarik merged commit 2e6b0cb into swiftlang:master Mar 5, 2019
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