Support @noescape SIL function types. #12420
Merged
Conversation
This file contains hidden or 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
|
@swift-ci benchmark. |
|
@swift-ci test. |
|
Build failed |
|
Build failed |
Build comment file:Optimized (O)Regression (1)
Improvement (4)
No Changes (329)
Unoptimized (Onone)Regression (2)
Improvement (12)
No Changes (320)
Hardware Overview |
|
Hi @atrick and @rjmccall, this looks promising! About +0 closure contexts, @jckarter already started working on that a while back but didn't finish it -- take a look at the code guarded by the EnableGuaranteedClosureContexts flag. Also, it would be nice if as part of doing this we could complete Joe's refactoring to change partial_apply to only apply a single context parameter, forming the partial apply forwarding thunk and closure context from a SILBoxType in SILGen. |
|
@slavapestov I'm just getting this branch in working order so @aschwaighofer can start working on the guaranteed context change (see EnableGuaranteedClosureContexts), and the stack allocation optimization. (https://bugs.swift.org/browse/SR-5441). I would also love to have the SILBoxType representation, along with a coulple other (non-ABI) changes to the closure representation like ditching |
|
Yeah, I think that the partial_apply improvements would be great but are not in any way blocking the use of non-escaping functions and callee_guaranteed contexts. |
|
@swift-ci test. |
|
Build failed |
|
Build failed |
|
When you convert_function to turn an escaping closure into a non-escaping one, will we actually emit a thunk in IRGen or are they ABI compatible? Agreed that partial_apply rework is not blocking this in any way and doesn't impact ABI, it would just be nice to do if it wasn't too much work. |
|
@swift-ci test. |
|
@slavapestov |
|
We would then need a different instruction in order to implement withoutActuallyEscaping (maybe just a partial_apply?), but I think that's reasonable. |
|
@swift-ci smoke test. |
|
@aschwaighofer, I've self-reviewed this PR and it passed testing and benchmarking. It should be ready for you to get started. I'll likely wait until Monday to merge so people have a little more time to review. |
|
The current implementation of |
|
@slavapestov yep, that's the idea for |
And set this attribute correctly when lowering formal function types to SILFunctionTypes based on @escaping. This will allow stack allocation of closures, and unblock a related ABI change.
we don't default it.
It might be better to use a specialized instruction here, but I'll leave that up to Andy. Andy: And I'll leave that to Arnold who is implementing SIL support for guaranteed ownership of thick function types.
Be conservative when combining convert_function. Most of our code doesn't know how to deal with function type mismatches yet.
Be conservative with function conversion. The inliner does not yet know how to cast arguments or convert between throwing forms.
|
@swift-ci test. |
|
Build failed |
|
Build failed |
|
@swift-ci test. |
|
I think this is causing a use-after-free in ASAN. |
|
@atrick Also, we are seeing failure in source compatibility suite https://ci.swift.org/job/swift-master-source-compat-suite/510 |
gparker42
pushed a commit
to gparker42/swift
that referenced
this pull request
Oct 18, 2017
This reverts commit d369aa4. It is failing in ASAN tests.
Merged
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.
These are the underlying SIL changes necessary to implement the new
closure capture ABI.
Note: This includes a change to function name mangling that
primarily affects reabstraction thunks.
The new ABI will allow stack allocation of non-escaping closures as a
simple optimization.
The new ABI, and the stack allocation optimization, also require
closure context to be @guaranteed. That will be implemented as the
next step.
Many SIL passes pattern match partial_apply sequences. These all
needed to be fixed to handle the convert_function that SILGen now
emits. The conversion is now needed whenever a function declaration,
which has an escaping type, is passed into a @NoEscape argument.
In addition to supporting new SIL patterns, some optimizations like
inlining and SIL combine are now stronger which could perturb some
benchmark results.
These underlying SIL changes should be merged now to avoid conflicting
with other work. Minor benchmark discrepancies can be investigated as part of
the stack-allocation work.