-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Conversation
@swift-ci test |
Build failed |
@swift-ci test linux |
There was a problem hiding this 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); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
0335efe
to
bd5d427
Compare
@swift-ci smoke test |
@swift-ci smoke test macOS |
1 similar comment
@swift-ci smoke test macOS |
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