Skip to content

[5.0] Fix SILCombine to devirtualize existentials wrapped in enum. #21239

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
Dec 12, 2018
Merged

[5.0] Fix SILCombine to devirtualize existentials wrapped in enum. #21239

merged 1 commit into from
Dec 12, 2018

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Dec 12, 2018

This allows Swift code to implement a fast path via a protocol type
check as follows:

if let existentialVal = genericVal as? SomeProtocol {
// do something fast.
}

Fixes rdar://problem/46322928 Failure to devirtualize a protocol
method applied to an opened existential blocks implemention of
DataProtocol.

Note: the approach of devirtualization via backward pattern matching
is fundamentally wrong and will never be fully general. It should be a
forward type propagation.

This allows Swift code to implement a fast path via a protocol type
check as follows:

  if let existentialVal = genericVal as? SomeProtocol {
    // do something fast.
  }

Fixes <rdar://problem/46322928> Failure to devirtualize a protocol
method applied to an opened existential blocks implemention of
DataProtocol.

Note: the approach of devirtualization via backward pattern matching
is fundamentally wrong and will never be fully general. It should be a
forward type propagation.
@atrick atrick requested a review from a team as a code owner December 12, 2018 03:02
@atrick
Copy link
Contributor Author

atrick commented Dec 12, 2018

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Dec 12, 2018

@swift-ci test source compatibility.

@atrick
Copy link
Contributor Author

atrick commented Dec 12, 2018

--- CCC ---

Explanation: Fix SILCombine to devirtualize Optional existentials. This blocks the addition of DataProtocol which is an ABI change.

Scope: Only affects -O builds. Extends an existing optimization to recognize more complex SIL patterns.

Issue: rdar://problem/46322928 Failure to devirtualize a protocol method applied to an opened existential blocks implemention of DataProtocol.

Risk: Medium. This adds a conservative pattern match to SILCombine. The biggest risk is that performing more optimization exposes some other bug in the optimizer.

Testing: New unit tests tests were added in both .swift and .sil.

@atrick
Copy link
Contributor Author

atrick commented Dec 12, 2018

@eeckstein please review for CCC.

I won't merge it until 5.0 branch is ready for PRs.

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!

SILValue swift::getAddressOfStackInit(AllocStackInst *ASI,
///
/// allocStackAddr may either itself be an AllocStackInst or an
/// InitEnumDataAddrInst that projects the value of an AllocStackInst.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense to turn this comment into an assert.

@atrick
Copy link
Contributor Author

atrick commented Dec 12, 2018

@swift-ci test source compatibility.

@eeckstein eeckstein merged commit d0d64a1 into swiftlang:swift-5.0-branch Dec 12, 2018
@atrick atrick deleted the 5.0-fix-devirt-enum branch July 31, 2019 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants