[AST/SIL] Expand SIL function type isolation to include nonisolated(nonsending)#88404
[AST/SIL] Expand SIL function type isolation to include nonisolated(nonsending)#88404xedin wants to merge 10 commits intoswiftlang:mainfrom
nonisolated(nonsending)#88404Conversation
|
@swift-ci please test |
|
@swift-ci please test |
Currently there is only `erased` but this is soon to gain one more - `nonisolated(nonsending)`.
…ation Similarly to `@isolated(any)` it's sometimes necessary to check whether a call is to a `nonisolated(nonsending)` function value i.e. for hop elimination pass and without this isolation support it has to go trying to figure out the isolation from a callee which is not possible if it doesn't refer to a `SILFunction *`.
…on in SIL function types
Generalizes how isolation is stored for SILFunctionType and makes it possible to store not just "erased" but any future isolation we'd need to add as well.
…ation If a function type requires an implicit isolation parameter also set `nonisolated(nonsending)` isolation on it. This helps us track all of the references to `nonisolated(nonsending)` functions.
|
@swift-ci please test |
jamieQ
left a comment
There was a problem hiding this comment.
A couple general interest questions for you if you have a moment @xedin:
- Why is this approach preferable over a narrower fix to just address the issue in the executor hop optimization pass?
- I assume the answer is "no", but is this expected to resolve the other apparent issue with that pass described in #88259?
| // Make sure that the invariant that `nonisolated(nonsending)` function | ||
| // always has an implicit leading parameter is maintained. |
There was a problem hiding this comment.
Not sure if it should be done in this PR, but do you think the verifier should enforce that the actor isolation and implicit leading parameters correspond in the expected way? Something like <is_nonisolated_nonsending_isolation> <-> <has_implicit_leading_isolated_param>? Or are there expected cases in which that does not hold?
There was a problem hiding this comment.
Yeah, I need to think about that some more that’s why I put an assert here for now.
| Buffer << 'A'; | ||
| break; | ||
| case Node::Kind::ImplNonisolatedNonsendingIsolation: | ||
| Buffer << 'N'; |
There was a problem hiding this comment.
Just out of curiosity, how are the letters for this encoding chosen?
There was a problem hiding this comment.
To minimize possibility of clashes but really arbitrarily, I used the first letter of the attribute and checked where else we use it.
|
@swift-ci please test Windows platform |
|
@jamieQ the reason why I did it this way is because there is no reliable way to make it narrow, we should track the isolation. |
rjmccall
left a comment
There was a problem hiding this comment.
Generally looks good, except I don't think we want the mangling suppression flag.
|
@swift-ci please test |
rjmccall
left a comment
There was a problem hiding this comment.
Approving because the patch seems overall fine, but I think we probably want to fix the printer.
…g)` isolation of SIL function type `nonisolated(nonsending)` is a modifier and it's really hard to work with in SIL because most of the things are represented as attributes there.
…allee Instead of trying to from isolation which is not always possible because that method replies on having a `SILFunction *` being called which is not the same for i.e. function values.
…implicit leading isolation parameter Doing so changes isolation because the invariant is that if the function is `@caller_isolated` it should have an implicit leading parameter.
…ted(nonsending)` mangling
|
@swift-ci please test |
|
@swift-ci please smoke test macOS platform |
|
@swift-ci please smoke test macOS platform |
1 similar comment
|
@swift-ci please smoke test macOS platform |
Explanation:
The primary driver for this change is the
OptimizeHopToExecutorpass because it has to check whether a call is a suspension point or not and the most convenient way to rule outnonisolated(nonsending)would be to check a callee but since thenonisolated(nonsending)isolation wasn't represented before it had to go through actor isolation which is not always easier to figure out from SIL. This means that some hops were optimized away even though they shouldn't have been.The change includes the following:
nonisolated(nonsending)to SILFunctionType;Nto represent the new isolation);getSILFunctionTypeis updated to start producing lowering withnonisolated(nonsending)isolation;FullyApplySite::isCallerIsolationInheritingis updated to check callee instead of reaching of the isolation;Resolves: [concurrency]: nonisolated(nonsending) can unexpectedly lose actor isolation #86083, rdar://169998321
Risk: Low to Medium. This is a representation change so it carries some risk of interactions with optimizations beyond specialization.
Testing: Added new test-cases to the suite, updated existing tests.