When a `borrowing` or `consuming` parameter is captured by a closure,
we emit references to the binding within the closure as if it is non-implicitly
copyable, but we didn't mark the bindings inside the closure for move-only
checking to ensure the uses were correct, so improper consumes would go
undiagnosed and lead to assertion failures, compiler crashes, and/or
miscompiles. Fixes rdar://127382105
After 874971c40d, before running address
checking, all lifetimes in the function are completed. That doesn't
quite work because lifetime completion expects not to encounter
reborrows or their adjacent phis.
This translates the rules for @preconcurrency import from SE-0337 into the
region isolation world. Specifically if a module is compiled without strict
concurrency checking and imported with @preconcurrency:
1. All types from that module that are implicitly non-Sendable have diagnostics
suppressed in swift 5 and swift 6.
2. All types from that module that are explicitly non-Sendable emit warnings in
both swift 5 and swift 6.
rdar://126804052
The reason why I am doing this is that I am going to be adding support for
preconcurrency imports to TransferNonSendable. That implies that we can have
preconcurrency import suppression in the SIL pipeline and thus that emitting the
diagnostic in Sema is too early.
To do this, I introduced a new module pass called
DiagnoseUnnecessaryPreconcurrencyImports that runs after the SILFunction pass
TransferNonSendable. The reason why I use a module pass is to ensure that
TransferNonSendable has run on all functions before we attempt to emit these
diagnostics. Then in that pass, we iterate over all of the modules functions and
construct a uniqued array of SourceFiles for these functions. Then we iterate
over the uniqued SourceFiles and use the already constructed Sema machinery to
emit the diagnostic using the source files.
rdar://126928265
When deallocating a partially allocated class in the trapping branch of
an unfailable cast, cast back down to the subclass which is being
partially deallocated before emitting the dealloc partial ref
instruction.
I have been using these in TransferNonSendable and they are useful in terms of
reducing the amount of code that one has to type to use this API. I am going to
need to use it in SILIsolationInfo, so it makes sense to move it into
SILOptimizer/Utils.
NFCI.
This is backing out an approach that I thought would be superior, but ended up
causing problems.
Originally, we mapped a region number to an immutable pointer set containing
Operand * where the region was tranferred. This worked great for a time... until
I began to need to propagate other information from the transferring code in the
analysis to the actual diagnostic emitter.
To be able to do that, my thought was to make a wrapper type around Operand
called TransferringOperand that contained the operand and the other information
I needed. This seemed to provide me what I wanted but I later found that since
the immutable pointer set was tracking TransferringOperands which were always
newly wrapped with an Operand *, we actually always created new pointer
sets. This is of course wasteful from a memory perspective, but also prevents me
from tracking transferring operand sets during the dataflow since we would never
converge.
In this commit, I fix that issue by again tracking just an Operand * in the
TransferringOperandSet and instead map each operand to a state structure which
we merge dataflow state into whenever we visit it. This provides us with
everything we need to in the next commit to including a region -> transferring
operand set equality check in our dataflow equations and always converge.
Compute, update and handle borrowed-from instruction in various utilities and passes.
Also, used borrowed-from to simplify `gatherBorrowIntroducers` and `gatherEnclosingValues`.
Replace those utilities by `Value.getBorrowIntroducers` and `Value.getEnclosingValues`, which return a lazily computed Sequence of borrowed/enclosing values.
It works well enough now that it should be an acceptable replacement for both
borrowing and consuming switches that works in more correct situations than the
previous implementation. This does however expose a few known issues that I'll
try to fix in follow ups:
- overconsumes cause verifier errors instead of raising diagnostics (rdar://125381446)
- cases with multiple pattern labels aren't yet supported (rdar://125188955)
- copyable types with the `borrowing` or `consuming` modifiers should probably use
noncopyable pattern matching.
The `BorrowingSwitch` flag is still necessary to enable the surface-level syntax
changes (switches without `consume` and the `_borrowing` modifier, for instance).
This PR implements first set of changes required to support autodiff for coroutines. It mostly targeted to `_modify` accessors in standard library (and beyond), but overall implementation is quite generic.
There are some specifics of implementation and known limitations:
- Only `@yield_once` coroutines are naturally supported
- VJP is a coroutine itself: it yields the results *and* returns a pullback closure as a normal return. This allows us to capture values produced in resume part of a coroutine (this is required for defers and other cleanups / commits)
- Pullback is a coroutine, we assume that coroutine cannot abort and therefore we execute the original coroutine in reverse from return via yield and then back to the entry
- It seems there is no semantically sane way to support `_read` coroutines (as we will need to "accept" adjoints via yields), therefore only coroutines with inout yields are supported (`_modify` accessors). Pullbacks of such coroutines take adjoint buffer as input argument, yield this buffer (to accumulate adjoint values in the caller) and finally return the adjoints indirectly.
- Coroutines (as opposed to normal functions) are not first-class values: there is no AST type for them, one cannot e.g. store them into tuples, etc. So, everywhere where AST type is required, we have to hack around.
- As there is no AST type for coroutines, there is no way one could register custom derivative for coroutines. So far only compiler-produced derivatives are supported
- There are lots of common things wrt normal function apply's, but still there are subtle but important differences. I tried to organize the code to enable code reuse, still it was not always possible, so some code duplication could be seen
- The order of how pullback closures are produced in VJP is a bit different: for normal apply's VJP produces both value and pullback closure via a single nested VJP apply. This is not so anymore with coroutine VJP's: yielded values are produced at `begin_apply` site and pullback closure is available only from `end_apply`, so we need to track the order in which pullbacks are produced (and arrange consumption of the values accordingly – effectively delay them)
- On the way some complementary changes were required in e.g. mangler / demangler
This patch covers the generation of derivatives up to SIL level, however, it is not enough as codegen of `partial_apply` of a coroutine is completely broken. The fix for this will be submitted separately as it is not directly autodiff-related.
---------
Co-authored-by: Andrew Savonichev <andrew.savonichev@gmail.com>
Co-authored-by: Richard Wei <rxwei@apple.com>
I fixed a bunch of small issues around here that resulted in a bunch of radars
being fixed. Specifically:
1. I made it so that we treat function_refs that are from an actor isolated
function as actor isolated instead of sendable.
2. I made it so that autoclosures which return global actor isolated functions
are treated as producing a global actor isolated function.
3. I made it so that we properly handle SILGen code patterns produced by
Sendable GlobalActor isolated things.
rdar://125452372
rdar://121954871
rdar://121955895
rdar://122692698
Emitting a note with an invalid source location is actively
harmful. It confuses users and tools, makes it impossible to write
unit tests. In this case, the note simply says "use here", so it's
completely free of information without the source location.
This fixes bugs when ~Escapable types depended on values that are passed to 'consume'.
The consume operator diagnostics are broken when dependent values are
present. This sidesteps the problem for lifetime dependence. And we
generally want to diagnose lifetime dependence after all move-only
related diagnostics. That way, using a dependent value after consume
provides a more informative diagnostic about the dependent value and
its scope.
Specifically, I added a named version of the diagnostic:
func testSimpleTransferLet() {
let k = Klass()
- transferArg(k) // expected-warning {{binding of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
+ transferArg(k) // expected-warning {{transferring 'k' may cause a race}}
+ // expected-note @-1 {{'k' used after being passed as a transferring parameter}}
useValue(k) // expected-note {{use here could race}}
}
and I also cleaned up the typed version of the diagnostic that is used
if we fail to find a name:
func testSimpleTransferLet() {
let k = Klass()
- transferArg(k) // expected-warning {{binding of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
- transferArg(k) // expected-warning {{value of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
useValue(k) // expected-note {{use here could race}}
}
This is the 2nd to the last part of a larger effort to rework all of
the region based diagnostics to first try and use names and only go back
to the old typed diagnostics when we fail to look up a name (which should
be pretty rare, but is always possible).
At some point if I really feel confident enough with the name lookup code, I am
most likely just going to get rid of the typed diagnostic code and just emit a
compiler doesnt understand error. The user will still not be able to ship the
code but would also be told to file a bug so that we can fix the name
inference.
As an example of the change:
- // expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
+ // expected-note @-1 {{transferring disconnected 'x' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
Part of the reason I am doing this is that I am going to be ensuring that we
handle a bunch more cases and I wanted to fix this diagnostic before I added
more incaranations of it to the tests.
