Sema: Rewrite partial applications into closures #28698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slavapestov
changed the title
slavapestov
force-pushed
the
sr-75
branch
3 times, most recently
from
7fb227c
to
6c1b4db
Compare
slavapestov
force-pushed
the
sr-75
branch
4 times, most recently
from
502789c
to
76dd4c9
Compare
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please benchmark |
|
Build failed |
|
Build failed before running benchmark. |
|
Build failed |
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
akyrtzi
reviewed
Dec 17, 2019
test/Index/roles.swift
Outdated
| let _ = AClass(x: 1).foo | ||
| // CHECK: [[@LINE-1]]:22 | instance-method/Swift | foo() | [[AClass_foo_USR]] | Ref | rel: 0 | ||
| // CHECK: [[@LINE-1]]:22 | instance-method/Swift | foo() | [[AClass_foo_USR]] | Ref,Call | rel: 0 |
There was a problem hiding this comment.
Marking these as "call sites" is inaccurate, could you correct this?
There was a problem hiding this comment.
This test passes with no changes in the latest version of the commit.
akyrtzi
reviewed
Dec 17, 2019
test/Index/roles.swift
Outdated
| @@ -352,7 +352,7 @@ let otherInstance = AStruct(x: 1) | |||
|
|
|||
| _ = AStruct.init(x:) | |||
| // CHECK: [[@LINE-1]]:18 | instance-property/Swift | x | [[AStruct_x_USR]] | Ref | rel: 0 | |||
| // CHECK: [[@LINE-2]]:5 | struct/Swift | AStruct | [[AStruct_USR]] | Ref | rel: 0 | |||
There was a problem hiding this comment.
Is the reference to AStruct in this line not reported anymore?
CodaFi
reviewed
Dec 17, 2019
lib/Sema/CSApply.cpp
Outdated
| return 2; | ||
| } else if (auto *elt = dyn_cast<EnumElementDecl>(member)) { | ||
| return elt->getParameterList() ? 2 : 1; |
There was a problem hiding this comment.
Can the brains of this function just be ValueDecl::getNumCurryLevels()? The assertions are still good to keep.
CodaFi
reviewed
Dec 17, 2019
lib/Sema/ConstraintSystem.cpp
Outdated
| Type type; | ||
| Type type = openedType; | ||
|
|
||
| if (!outerDC->getSelfProtocolDecl()) { |
There was a problem hiding this comment.
This is like the third or fourth time this block appears in the codebase. Can we centralize this? One of them is going to fall out of sync one day...
There was a problem hiding this comment.
I only found two, but I'll extract it into a new method on ValueDecl.
CodaFi
approved these changes
Dec 18, 2019
Try to make this a bit more self-documenting.
|
@swift-ci Please benchmark |
|
@swift-ci Please test source compatibility |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci Please benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
When a method is called with fewer than two parameter lists,
transform it into a fully-applied call by wrapping it in a
closure.
Eg,
Foo.bar => { self in { args... self.bar(args...) } }
foo.bar => { self in { args... self.bar(args...) } }(self)
super.bar => { args... in super.bar(args...) }
With this change, SILGen only ever sees fully-applied calls,
which will allow ripping out some code.
This new way of doing curry thunks fixes a long-standing bug
where unbound references to protocol methods did not work.
This is because such a reference must open the existential
*inside* the closure, after 'self' has been applied, whereas
the old SILGen implementation of curry thunks really wanted
the type of the method reference to match the opened type of
the method.
A follow-up cleanup will remove the SILGen curry thunk
implementation.
Fixes rdar://21289579 and https://bugs.swift.org/browse/SR-75.
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux |
|
|
||
| * [SR-75][]: | ||
|
|
||
| Unapplied references to protocol methods methods are now supported. Previously this |
There was a problem hiding this comment.
methods methods ➡️ methods
-Unapplied references to protocol methods methods are now supported. Previously this
+Unapplied references to protocol methods are now supported. Previously this| func play(catToy: Toy) | ||
| } | ||
|
|
||
| let fn = Cat.play |
There was a problem hiding this comment.
Require parameter names when referencing to functions?
-let fn = Cat.play
+let fn = Cat.play(catToy:)There was a problem hiding this comment.
Not actually required, but I agree it makes the example more clear.
|
Hi @slavapestov – Congratulations on landing this and fixing a long standing issue. That's awesome! Do you know of (or foresee) any case where a |
|
@davezarzycki That's a good question. It may very well be that the answer is "no". Do you have a further simplification in mind? |
|
I don’t have a simplification in mind although it does seem like it could be simplified. FWIW – This change broke a research branch I’m working on and I wanted to know how paranoid my misc diagnostic should be after this pull request. I think I know now. Thanks |
This was referenced Apr 15, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes rdar://21289579 / https://bugs.swift.org/browse/SR-75.
There's a big follow-up cleanup, which is removing curry thunks and assorted machinery from SILGen. I'll land that separately.