When a address-only noncopyable value is dead-def'ed by an indirect return from a `try_apply`,
the cleanup should be inserted on the normal return successor block. Fixes rdar://118255228.
When promoting a load_borrow, the re-borrows were not considered which lead to leaked values.
Now, just bail if a load_borrow has re-borrows.
rdar://118402432
Currently when one says that an instruction is not a "look through" instruction,
each of its results gets a separate element number and we track these results as
independent entities that can be in a region. The one issue with this is
whenever we perform this sort of operation we actually are at the same time
performing a require on the operand of the instruction. This causes us to emit
errors on non-side effect having instructions when we really want to emit an
error on their side-effect having results. As an example of the world before
this patch, the following example would force the struct_element_addr to have a
require so we would emit an error on it instead of the apply (the thing that we
actually care about):
```
%0 = ...
// We transferred %0, so we cannot use it again.
apply %transfer(%0)
// We track %1 and %0 as separate elements and we treat this as an assignment of
// %0 into %1 which forces %0 to be required live at this point causing us to
// emit an error here...
%1 = struct_element_addr %0
// Instead of in the SIL here on the actual side-effect having instruction.
apply %actualUse(%1)
```
the solution is to make instructions like struct_element_addr lookthrough
instructions which force their result to just be the same element as their
operand. As part of doing this, we have to ensure that getUnderlyingTrackedValue
knows how to look through these types. This ensures that they are not considered
roots.
----
As an aside to implement this I needed to compose some functionality ontop of
getUnderlyingObject (specifically the look through behavior on destructures) in
a new helper routine called getUnderlyingTrackedObjectValue(). It just in a loop
calls getUnderlyingObject() and looks through destructures until its iterator
doesn't change.
getExprForPartitionOp(...) just returned the expression from the loc of op.currentInst:
SILInstruction *sourceInstr = op.getSourceInst(/*assertNonNull=*/true);
Expr *expr = sourceInstr->getLoc().getAsASTNode<Expr>();
Instead of mucking around with exprs, just use the SILLocation from the
SILInstruction.
I also changed how we unique transfer instructions to just use the transfer
instruction itself instead of the AST/Expr of the transfer instruction.
Was experimenting with making PartitionOps a noncopyable type and I discovered
these places where we copy PartitionOps when we could use a const reference. It
is good not to copy PartitionOps since they potentially contain a heap allocated
array.
Sadly, my change to make PartitionOps noncopyable will have to wait until a
forthcoming commit here I overhaul how we emit errors since that older code
copies PartitionOps a lot and I would rather just delete that code and then fix
PartitionOps. But these are on the surface safe changes that makes sense to get
in separately to make that next patch easier to review.
What this does is really split the one dataflow we are performing into two
dataflows we perform at the same time. The first dataflow is the region dataflow
that we already have with transferring never occurring. The second dataflow is a
simple gen/kill dataflow where we gen on a transfer instruction and kill on
AssignFresh. What it tracks are regions where a specific element is transferred
and propagates the region until the element is given a new value. This of course
means that once the dataflow has converged, we have to emit an error not if the
value was transferred, but if any value in its region was transferred.
There isn't a strong reason to use a callback here since we aren't ever
composing transformations on partition ops. Better instead to go for simplicity
and just iterate directly. If we start doing complex transformations over
partition ops with composable APIs, we can always add this back in later. As a
nice benefit, one doesn't need to worry that the callback API is hiding actual
complexity... since just by using a for loop we communicate that nothing
interesting is happening here.
Just reducing the amount of code surface area in the pass.
This is only used in one place in partition analysis which is a data structure
that does computation. In contrast, we want BlockPartitionState to be more of a
POD type of data that each BasicBlock has mapped to it.
Simplifying the code.
This also let me get rid of the translator field in BasicBlockState. We only
need to pass it in as an argument to the constructor to initialize our
translation. It doesn't need to be stored anymore.
PR #69652 protected one call of `printID` but left another two in the
file. Create two small lambdas to print the ID with `printID` or just
print `NOASSERTS` depending on `NDEBUG` being defined. Change all the
callsites of `printID` to use that lambda.
I also included changes to the rest of the SIL optimizer pipeline to ensure that
the part of the optimizer pipeline before we lower tuple_addr_constructor (which
is right after we run TransferNonSendable) work as before.
The reason why I am doing this is that this ensures that diagnostic passes can
tell the difference in between:
```
x = (a, b, c)
```
and
```
x.0 = a
x.1 = b
x.2 = c
```
This is important for things like TransferNonSendable where assigning over the
entire tuple element is treated differently from if one were to initialize it in
pieces using projections.
rdar://117880194
The reason that this is being done is that since currently region based
isolation is not field sensitive, when we assign to the struct or tuple field of
the var, the new region relationship is set for the entire struct, not just a
specific field. This means that we effectively lose any region information from
the other fields. For example in the following at (1), given the current rules, we
lose that s.y was assigned to x:
```swift
struct S {
var x: NonSendableKlass
var y: NonSendableKlass
}
func foo() {
var s = S()
// Regions: [s]
let x = NonSendableKlass()
let y = NonSendableKlass()
// Regions: [(s), (x), (y)]
s.y = x
// Regions: [(s, x), (y)]
s.x = y (1)
// Regions: [(x), (s, y)]
}
```
The best solution to this is to track such var like bindings in a field
sensitive manner where the regions of the aggregate are the union of the
individual fields. This would let us represent the regions of the above as
follows:
```swift
func foo() {
var s = S()
// Regions: [((s.x), (s.y))]
let x = NonSendableKlass()
let y = NonSendableKlass()
// Regions: [((s.x), (s.y)), (x), (y)]
s.y = x
// Regions: [((s.x), (s.y, x)), (y)]
s.x = y (1)
// Regions: [((s.x, y), (s.y, x))]
}
```
We cannot do this today so to plug this hole, we instead treat these operations
as merges. This provides a conservative answer. Thus we would have:"
```swift
func foo() {
var s = S()
// Regions: [s]
let x = NonSendableKlass()
let y = NonSendableKlass()
// Regions: [(s), (x), (y)]
s.y = x
// Regions: [(s, x), (y)]
s.x = y (1)
// Regions: [(s, x, y])
}
```
This is because we are treating the assignment into s.y and s.x as merges into
s, so we do not lose that y was assigned into s before we assigned y into
it. The unfortunate side-effect of this is that if a struct or tuple has
multiple fields, the merge makes it so that if we assign over the same field, we
do not lose the region of the old value:
```swift
func foo() {
var s = S()
// Regions: [s]
let x = NonSendableKlass()
let y = NonSendableKlass()
// Regions: [(s), (x), (y)]
s.y = x
// Regions: [(s, x), (y)]
s.y = y (1)
// Regions: [(s, x, y])
}
```
If we were not to do this, then the s.y at (1) would result in s and x being in
different regions.
rdar://117273952
Ensuring that conformances to such protocols must have their type metadata always emitted into the binary, regardless of wheter they are used or `public`.
This gives me more control in getUnderlyingTrackedValue and also will allow me
to define another function that can detect if we see a projection on an address
so I can change it into merges in the next commit.
Currently when we create an assign instruction, if we find that the result of
the instruction and the operand of the instruction reduce to the same element
representative, then we do not actually emit an assign.
For certain instructions this makes sense, but this is misleading for
instructions like copies (copy_value) and geps (struct_element_addr) that this
is always true for. Instead of attempting to assign and just have the builder
always clean this up... make it explicit with a new routine called
translateSILLookThrough. When this is called, we just look up the value and
assert.
Specifically:
1. I changed Partition::apply so that it has an emitLog flag. The reason why I
did this is we run apply in a few different situations sometimes when we want to
emit logging other times when we really don't. For instance, we want to emit
logging when walking instructions and updating the entry partition. On the other
hand, we do not want to emit logging if we apply a value to a partition while
attempting to determine why an error needed to be emitted.
2. When we create an assign partition op and we see that our destination and
source are the same representative, we do not create the actual assign. Before
we did not log this so it looked like there was a logic error that was stopping
us from emitting a partition op when visiting said instructions. Now, we emit a
small logging message so it isn't possible to be confused.
3. Since I am adding another parameter to Partition::apply, I decided to
refactor Partition::apply to be in a separate PartitionOpEvaluator data
structure that contains the options that we used to pass into Partition::apply.
This prevents any mistakes around configuring Partition::apply since the fields
provide nice names/common sense default values.
I am going to be adding a second flag to be passed into translateMultiAssign.
Having multiple boolean flags can lead to confusion and mistakes when using the
API, so before I do that I am using this as an NFC commit to convert the API to
take an option set so the options are explicit.
The reason for this issue is that we were originally checking in our NonSendable
type oracle just if a type conformed to Sendable. But function types do not
conform to protocols, so this would fail for protocols. To fix this, I added
some helper methods to call swift::isSendableType on SILType, called that in our
oracle, and then I added support in swift::isSendableType for both
SILFunctionType and AnyFunctionType so that we correctly handle them depending
on their sendability.
There was also a question if we also handled function conversions correctly. I
added a test case that shows that we do not error in either of the cases.
Another nice aspect of this change is that we no longer need to stash a pointer
to a looked up Sendable protocol to perform the check since that just happens
naturally inside SILType::isSendable() when it calls isSendableType. This is a
better separation of concerns.
rdar://116525224
We are going to visit all of the blocks anyways... so it makes sense to just run
them as part of the initializing of the block state. Otherwise, the logic is
buried in the dataflow which makes it harder for someone who is not familiar
with the pass to reason about it.
If we treat a begin_access as an assignment then in cases like below we assume
that a value was used before we reassign.
```
func testVarReassignStopActorDerived() async {
var closure = {}
await transferToMain(closure)
// This re-assignment shouldn't error.
closure = {} // (1)
}
```
rdar://117437299
We only ever used this cache of IDs to construct the function arg
partition. Since a Partition involves using memory, it makes sense to just cache
the Partition itself and eliminate the unnecessary second cache.
