When merging SILIsolationInfo for regions, we want to drop
nonisolated(unsafe). This is important since nonisolated(unsafe) should only
apply to the specific "value" that it belongs to, not the entire region.
This creates a problem since in a few places in the code base we initialize a
value (producing a disconnected value) and then initialize it by merging in an
actor isolation. This no longer work since we will then always have
nonisolated(unsafe) stripped, so no values would ever be considered to be
nonisolated(unsafe). After analyzing the use case, I realized that these were
just initialization patterns and in this commit, I added a specific
initialization operation called SILIsolationInfo::initializeTrackableValue and
eliminated those calls to SILIsolationInfo::mergeIsolationRegionInfo.
Since SILIsolationInfo no longer has any merge operation on it, I then
eliminated that code in this commit. This completes the behavior split that I
put into the type system in the last commit. Specifically, I defined a
composition type called SILDynamicMergedIsolationInfo. It represents a
SILIsolationInfo that has been merged... that is why I called it the
DynamicMergedIsolationInfo. It could probably use a better name = (.
This fixes one of the last weird test case that I wrote where we were not letting through valid
nonisolated(unsafe) code.
At the same time, I discovered an additional issue (which can be seen in the
TODOs in this commit), where we are being too conservative around a non-Sendable
class var field. I am going to fix that in the next commit.
rdar://128299305
DISCUSSION: The analysis itself is unable to emit errors. So we achieve the same
functionality by in such cases emitting a partition op that signals to our user
that when they process that partition op they should emit an "unknown pattern"
error at the partition op's instructions.
I have wanted this for a long time, but I never got around to it.
I made sure we match what we get without region isolation by turning off region
isolation in one of the test runs on the test for this.
There is one problem where for non-final classes with nonisolated(unsafe) var
fields, we currently do not properly squelch since I need to do more
infrastructure work. I am going to do that in the next commit.
rdar://128299305
The design change here is that instead of just initializing the regionInfo with
disconnected, we set it as .none and if we see .none, just return a newly
construct disconnected isolation region info when getIsolationRegionInfo() is
called.
This enables us to provide a setIsolationRegionInfo() helper for
RegionAnalysisValueMap::getTrackableValue that does not perform a merge. This is
important since for nonisolated(unsafe), we want to not have nonisolated(unsafe)
propagate through merging. So if we use merging to initialize the internal
regionInfo state of a SILIsolationInfo, we will never have a SILIsolationInfo
with that bit set since it will be lost in the merge. So we need some sort of
other assignment operator. Noting that we should only compute a value's
SILIsolationInfo once in RegionAnalysisValueMap before we cache it in the map,
it made sense to just represent it as an optional that way we can guarantee that
the regionInfo is only ever set exactly once by that routine.
There is no guarantee that these other helpers properly model lookthrough as our
model does. This ensures that this routine is always in sync with how we define
lookthrough in our model.
The problem with the old approach can be seen in how we handled move_value. The
model and the later code knew correctly that they should not look through
move_value that is marked as [var_decl]. But this other analysis code did not.
This with the tree today should not have any impact. But with the fix I am doing
now (fixing nonisolated(unsafe)) and later isolation history this will become a
problem.
We still only parse transferring... but this sets us up for adding the new
'sending' syntax by first validating that this internal change does not mess up
the current transferring impl since we want both to keep working for now.
rdar://128216574
This is safe since:
1. We transfer in the non-Sendable parameter into the global actor isolation
region so we know that we will not use the non-Sendable paramter again except on
that actor.
2. Since the closure is global actor isolated, we know that despite the fact
that it is Sendable, it will only ever be executed serially on said global actor
implying that we do not need to worry about different executions of the Sendable
closure running concurrently with each other.
rdar://125200006
As part of this I went through how we handled inference and rather than using a
grab-bag getActorIsolation that was confusing to use, I created split APIs for
specific use cases (actor instance, global actor, just an apply expr crossing)
that makes it clearer inside the SILIsolationInfo::get* APIs what we are
actually trying to model. I found a few issues as a result and fixed most of
them if they were small. I also fixed one bigger one around computed property
initializers in the next commit. There is a larger change I didn't fix around allowing function
ref/partial_apply with isolated self parameters have a delayed flow sensitive
actor isolation... this will be fixed in a subsequent commit.
This also fixes a bunch of cases where we were printing actor-isolated instead
of 'self' isolated.
rdar://127295657
This ensures that when we process, we consider load/load_borrow's result to be
equivalent to its operand. This ensures that a load/load_borrow cannot act as a
use of its operand preventing spurious diagnostics.
The reason why we do this is that we want to treat this as a separate value from
their operand since they are the result of defining a new value.
This has a few nice side-effects, one of which is that if a let results in just
a begin_borrow [var_decl], we emit names for it.
I also did a little work around helping variable name utils to lookup names from
applies that are fed into a {move_value,begin_borrow} [var_decl] which then has
the debug_value we are searching for.
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.
This ensures that we can efficiently iterate over the map which we will need to
do for equality queries.
I am going to add the equality queries in a subsequent commit. Just chopping off
a larger commit.
Specifically:
1. I copy the history that we have been tracking from the transferring operand
value at the transfer point. This is then available for use to emit diagnostics.
2. I added the ability for SILIsolationInfo to not only track the ActorIsolation
of an actor isolated value, but also if we have a value, we can track that as
well. Since we now track a value for task isolated and actor isolated
SILIsolationInfo, I just renamed the field to isolatedValue and moved it out of
the enum.
In a subsequent commit, I am going to wire it up to a few diagnostics.
rdar://123479934
We package all isolation history nodes from a single instruction by placing a
sequence boundary at the bottom. When ever we pop, we actually pop a PartitionOp
at a time meaning that we pop until we see a SequenceBoundary. Thus the sequence
boundary will always be the last element visited when popping meaning that it is
a convenient place to stick the SILLocation associated with the entire
PartitionOp. As a benefit, there was some unused space in IsolationHistory::Node
for that case since we were not using the std::variant field at all.
This means that I added an IsolationHistory field to Partition. Just upstreaming
the beginning part of this work. I added some unittests to exercise the code as
well. NOTE: This means that I did need to begin tracking an
IsolationHistoryFactory and propagating IsolationHistory in the pass
itself... but we do not use it for anything.
A quick overview of the design.
IsolationHistory is the head of an immutable directed acyclic graph. It is
actually represented as an immutable linked list with a special node that ties
in extra children nodes. The users of the information are expected to get a
SmallVectorImpl and process those sibling nodes afterwards. The reason why we
use an immutable approach is that it fits well with the problem and saves space
since different partitions could be pointing at the same linked list
node. Operations occur on an isolation history by pushing/popping nodes. It is
assumed that the user will push nodes in batches with a sequence boundary at the
bottom of the addition which signals to stop processing nodes.
Tieing this together, each Partition within it contains an IsolationHistory. As
the PartitionOpEvaluator applies PartitionOps to Partition in
PartitionOpEvaluator::apply, the evaluator also updates the isolation history in
the partition by first pushing a SequenceBoundary node and then pushing nodes
that will undo the operation that it is performing. This information is used by
the method Partition::popHistory. This pops linked list nodes from its history,
performing the operation in reverse until it hits a SequenceBoundary node.
This allows for one to rewind Partition history. And if one stashes an isolation
history as a target, one can even unwind a partition to what its state was at a
specific transfer point or earlier. Once we are at that point, we can begin
going one node back at a time and see when two values that we are searching for
no longer are apart of the same region. That is a place where we want to emit a
diagnostic. We then process until we find for both of our values history points
where they were the immediate reason why the two regions merge.
rdar://123479934
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>
This should be NFC since the only case where I used this was with self... and I
found another way of doing that using the API I added in the previous commit.
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
This issue can come up when a value is initially statically disconnected, but
after we performed dataflow, we discovered that it was actually actor isolated
at the transfer point, implying that we are not actually transferring.
Example:
```swift
@MainActor func testGlobalAndGlobalIsolatedPartialApplyMatch2() {
var ns = (NonSendableKlass(), NonSendableKlass())
// Regions: (ns.0, ns.1), {(mainActorIsolatedGlobal), @MainActor}
ns.0 = mainActorIsolatedGlobal
// Regions: {(ns.0, ns.1, mainActorIsolatedGlobal), @MainActor}
// This is not a transfer since ns is already main actor isolated.
let _ = { @MainActor in
print(ns)
}
useValue(ns)
}
```
To do this, I also added to SILFunction an actor isolation that SILGen puts on
the SILFunction during pre function visitation. We don't print it or serialize
it for now.
rdar://123474616
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.
I am making this specific API since I am going to make it so that
SILIsolationInfo::get(SILInstruction *) can infer isolation info from self even
from functions that are not apply isolation crossing points. For example, in the
following, we need to understand that test is main actor isolated and we
shouldn't emit an error.
```swift
@MainActor func test(_ x: NonSendable) {}
@OtherActor func doSomething() {
let x = NonSendable()
Task.init { @MainActor in print(x) }
test(x)
}
```
Long term I would like to get region analysis and transfer non sendable out of
the business of directly interpreting the AST... but if we have to do it now, I
would rather us do it through a helper struct. At least the helper struct can be
modified later to work with additional SIL concurrency support when it is added.
I also eliminated the very basic "why is this task isolated" part of the warning
in favor of the larger, better, region history impl that I am going to land
soon. The diagnostic wasn't that good in the first place and also was fiddly. So
I removed it for now.
rdar://124960994
The reason why I am doing this is that:
1. function_extract_isolation can take as a parameter a non-Sendable function
today in SIL so in such a case, we crash.
2. It returns an Optional<any Actor> which always must be Sendable.
So it makes sense for it to just require that its non-Sendable parameter not be
transferred at that point.
This assert validates that look through parameters only have a single operand
(the one we are going to lookthrough). It did not take into account though that
some of these lookthrough instructions /do/ have type dependent operands (which
we should ignore for the assert). I changed the assert to ignore those.
The specific problem is that instead of just using the parent type of the
ref_element_addr (which is guaranteed to be a class), we used the base of the
storage... the base doesn't have to be the ref_element_addr's operand. The
crasher occured when that base was a class existential that we cast to a super
class whose field we access.
