Skip to content

Devirtualizer: disable the “effectively final” optimization if the caller is serialized #17327

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
Jun 19, 2018

Conversation

shajrawi
Copy link

rdar://problem/41106812

Improve the check done in #14740

The class_method being devirtualized might be OK for most call sites, i.e. we are allowed to devirtualize it, however, the caller itself might have been marked with inlinable

…ller is serialized

Improve the check done in swiftlang#14740

The class_method being devirtualized might be OK for most call sites, i.e. we are allowed to devirtualize it, however, the caller itself might have been marked with inlinable
@shajrawi
Copy link
Author

@swift-ci Please smoke test and merge

@swift-ci swift-ci merged commit 7d2a75d into swiftlang:master Jun 19, 2018
@@ -516,7 +516,9 @@ bool swift::canDevirtualizeClassMethod(FullApplySite AI,

// We need to disable the “effectively final” opt if a function is inlinable
if (isEffectivelyFinalMethod &&
F->getResilienceExpansion() == ResilienceExpansion::Minimal) {
((F->getResilienceExpansion() == ResilienceExpansion::Minimal) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first check even necessary? It seems like this is always about the call site. cc @slavapestov

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @jrose-apple. It's always OK to inline an inlinable function into a non-inlinable function.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the first check: #17337

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