Skip to content

SILCombine: fix keypath optimization with optional chaining and classes #37166

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
May 3, 2021

Conversation

eeckstein
Copy link
Contributor

For optional chaining a swift_enum is created.
If the sub-projection is ending a begin_access in the some-branch, it also needs to be ended in the none-branch.

Fixes a compiler crash and/or a miscompile.

https://bugs.swift.org/browse/SR-14534
rdar://77224220

@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein eeckstein assigned atrick and unassigned atrick Apr 30, 2021
@eeckstein eeckstein requested a review from atrick April 30, 2021 07:50
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0335efef8e4568e9aa9119cb8311917fd32cddf5

@eeckstein
Copy link
Contributor Author

@swift-ci test linux

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.

Interesting bug. Thanks for the fix.

I have a couple of comments inline.

@@ -230,7 +230,7 @@ class GettablePropertyProjector : public ComponentProjector {
auto addr = builder.createAllocStack(loc, type);

assertHasNoContext();
assert(getter->getArguments().size() == 2);
assert(!getter->isDefinition() || getter->getArguments().size() == 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the assertion loosened? Instead of getter->getConventions().getNumSILArguments()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it

sil @kpsetter : $@convention(thin) (@in_guaranteed String, @in_guaranteed Kp2) -> ()
sil @swift_getAtKeyPath : $@convention(thin) <τ_0_0, τ_0_1> (@in_guaranteed τ_0_0, @guaranteed KeyPath<τ_0_0, τ_0_1>) -> @out τ_0_1

// CHECK-LABEL: sil @test_optional_chaining_keypath
Copy link
Contributor

Choose a reason for hiding this comment

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

I would never guess what that test is supposed to do. You can just paste the relevant source lines from the bug and explain how SILCombine lowers this optional keypath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment

For optional chaining a swift_enum is created.
If the sub-projection is ending a begin_access in the some-branch, it also needs to be ended in the none-branch.

Fixes a compiler crash and/or a miscompile.

https://bugs.swift.org/browse/SR-14534
rdar://77224220
@eeckstein eeckstein force-pushed the fix-keypath-optimization branch from 0335efe to bd5d427 Compare April 30, 2021 19:48
@eeckstein
Copy link
Contributor Author

@swift-ci smoke test

@eeckstein
Copy link
Contributor Author

@swift-ci smoke test macOS

1 similar comment
@eeckstein
Copy link
Contributor Author

@swift-ci smoke test macOS

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