Apply the MoveOnlyAddressChecker change from
https://github.com/swiftlang/swift/pull/73358 to the
MoveOnly[Value]Checker.
After 7713eef817, before running value
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.
rdar://151325025
Now that the underlying issue (PrunedLiveness' merging of summaries for
branch instructions) has been fixed, reinstate lifetime completion and
add a test to verify that it behaves correctly.
This reverts commit c552b90b61
("Temporarily turn off completing lifetimes of block arguments").
rdar://130427564
- While an opaque borrow access occurs to part of a value, the entire scope of
the access needs to be treated as a liveness range, so add the `EndAccess`es
to the liveness range.
- The SIL verifier may crash the compiler on SILGen-generated code when the
developer's source contains consume-during-borrow code patterns. Allow
`load_borrow` instructions to be marked `[unchecked]`, which suppresses
verifier checks until the move checker runs and gets a chance to properly
diagnose these errors.
Fixes rdar://124360175.
Although I don't plan to bring over new assertions wholesale
into the current qualification branch, it's entirely possible
that various minor changes in main will use the new assertions;
having this basic support in the release branch will simplify that.
(This is why I'm adding the includes as a separate pass from
rewriting the individual assertions)
I am doing this since region based isolation hit the same issue that the move
checker did. So it makes sense to refactor the functionality into its own pass
and move it into a helper pass that runs before both.
It is very conservative and only stubifies functions that the specialization
passes explicitly mark as this being ok to be done to.
Before move-checking values, complete the lifetimes of all the values
derived from them via copy, borrow, and move.
Collect all such values and their derived transitive values and then
complete the lifetimes of each, visiting the instructions which produce
them in post-order.
Once OSSALifetimeCommpletion runs as part of SILGenCleanup, this code
can be deleted.
I was originally hoping to reuse mark_must_check for multiple types of checkers.
In practice, this is not what happened... so giving it a name specifically to do
with non copyable types makes more sense and makes the code clearer.
Just a pure rename.
llvm::SmallSetVector changed semantics
(https://reviews.llvm.org/D152497) resulting in build failures in Swift.
The old semantics allowed usage of types that did not have an
`operator==` because `SmallDenseSet` uses `DenseSetInfo<T>::isEqual` to
determine equality. The new implementation switched to using
`std::find`, which internally uses `operator==`. This type is used
pretty frequently with `swift::Type`, which intentionally deletes
`operator==` as it is not the canonical type and therefore cannot be
compared in normal circumstances.
This patch adds a new type-alias to the Swift namespace that provides
the old semantic behavior for `SmallSetVector`. I've also gone through
and replaced usages of `llvm::SmallSetVector` with the
`Swift::SmallSetVector` in places where we're storing a type that
doesn't implement or explicitly deletes `operator==`. The changes to
`llvm::SmallSetVector` should improve compile-time performance, so I
left the `llvm::SmallSetVector` where possible.
This is an improvement of #67031 which avoids deleting the closure function
body during AllocBoxToStack, which still breaks pass invariants by modifying
functions other than the currently-analyzed function. As a function pass,
AllocBoxToStack also doesn't really know with certainty whether the original
closure function is unused after stack promotion or not. We still want to
eliminate the original when it may contain invalid SIL for move-only values
that rely on the escape analysis for correct semantics, so rather than mark the
original function to be *ignored* during move-only checking, mark it to be
*deleted* by move-only checking if the function is in fact unused at that
point.
If the marked function is still used, we let it pass through move-only
checking normally, which may cause redundant diagnostics but is the right
thing to do since code is still potentially using the closure with escaping
semantics. We should rearrange things to make this situation impossible in
the future.
rdar://110675352
Previously, we would emit a "compiler doesn't understand error" since we would
detect the escape and fail. That is the correct behavior here if the
partial_apply is not already identified as escaping by an earlier pass. But in
the case where we see that the partial_apply's callee was marked with
semantics::NO_MOVEONLY_DIAGNOSTICS, then we:
1. Suppress the "compiler doesn't understand error" for this specific
mark_must_check.
2. Suppress function wide the "copy of noncopyable type" error. Since we stopped
processing the mark_must_check that was passed to the partial_apply, we may
have left copies of noncopyable types on that mark_must_check value. This is
ok since the user will get the error, will recompile, and if any further show
up after they fix the inout escaping issue, they will be emitted
appropriately.
The reason why I am doing this is that currently SILGen knows when emitting said
closure that we are going to emit an error (that it does not have enough
information to emit itself since it doesn't know the caller), so doesn't emit
move checking markers. The result is that the move checker will not eliminate
any copies in the closure and thus will emit a "copy of noncopyable type found"
error and tell the user to file a bug. This just suppresses that.
rdar://108511866
This is the first slice of bringing up escaping closure support. The support is
based around introducing a new type of SILGen VarLoc: a VarLoc with a box and
without a value. Because the VarLoc only has a box, we have to in SILGen always
eagerly reproject out the address from the box. The reason why I am doing this
is that it makes it easy for the move checker to distinguish in between
different accesses to the box that we want to check separately. As such every
time that we open the box, we insert a mark_must_check
[assignable_but_not_consumable] on that project. If allocbox_to_stack manages to
determine that the box can be stack allocated, we eliminate all of the
mark_must_check and place a new mark_must_check [consumable_and_assignable] on
the alloc_stack. The end result is that we get the old model that we had before
and also can support escaping closures.
Otherwise, sometimes when the object checker emits a diagnostic and cleans up
the IR, some of the cleaned up copies are copies that should have been handled
by the address checker. The end result is that the address checker does not emit
diagnostics for that IR. I found this problem was exascerbated when writing code
for escaping closures.
This commit also cleans up the passes in preparation for at a future time moving
some of the transformations into the utils folder.
This is just to get code coverage. A few interesting things I noticed:
1. I am seeing a false diagnostic on this pattern:
let x = ...
var x2 = x
print(x2) // I get that print(x2) is a consuming use of x.
I think it is predictable mem opts maybe forwarding in an interesting way. I am
not sure. It doesn't happen with no implicit copy types since x2 would not be no
implicit copy so we would leave the no implicit copy monad.
2. I think that SILGen creates a real lexical scope for temporary variables
meaning that compiler generated code would actually hit the move checker. I had
to add an extra code pattern to handle this.
The reason why we are doing this is that we are really accessing a getter on the
type. The error would be necessarily done inside the getter where any
consumption would happen. So it shouldn't be on the move only value itself.
This is just the very beginning... I still need to implement more parts of
SILGen for this. But all great things start small. I am going to iterate on top
of this and just wanted to get some initial parts of the work in as I go.
We do the same thing already with the move function. If someone is using this
functionality and the compiler fails, we don't want to succeed the compilation
since the user is relying on the compiler for safety!
This involved doing the following:
1. Update the move only checker to look for new patterns.
2. Teach emitSemanticStore to use a moveonlywrapper_to_copyable to store moveonly
values into memory. The various checkers will validate that this code is
correct.
3. When emitting an apply, always unwrap move only variables. In the
future, I am going to avoid this if a parameter is explicitly marked as also
being moveonly (e.x.: @moveOnly parameter or @noEscape argument).
4. Convert from moveOnly -> copyable on return inst automatically in SILGen.
5. Fix SILGenLValue emission so we emit an error diagnostic later rather than
crash. This is needed to keep SILGen emitting move only addresses (that is no
implicit copy address only lets) in a form that the move only checker then
will error upon. Without this change, SILGen crashes instead of emitting an
error diagnostic in the following test:
.//test/SILOptimizer/move_only_checker_addressonly_fail.swift
Example: consume(x.field). This turned out to be a pretty simple extension of
the underlying model. The cases we are interested in are caused by a
non-reference nominal type having an extracted field being passed to a consuming
use. This always requires a copy.
The reason I missed this was I originally wrote the test cases around this for
classes which do not have this problem since the class is move only, not the
field due to class being a reference type. I then cargo culted this test case
for struct/other types and did not notice that we should have started to error
on these tests.
On an interesting note, I caught this on my branch where I am preparing the
optimizer to allow for values to have a move only bit. One of the constraints is
that once we are in guaranteed SIL, copy_value can not be used on any moveOnly
type (e.x.: $@moveOnly T). To ensure this doesn't happen, the move only checker:
1. Uses copy propagation to rewrite the copies of the base owned value.
2. Emit a diagnostic error upon any copies we found were needed due to the owned
value being consumed.
3. If a diagnostic was emitted, rewrite all copies of move only typed values to
be explicit_copy_value to ensure that in canonical SIL we do not violate the
invariant that copy_value can not be applied to move only type values. This
is ok to do since we are going to error and just want to avoid breaking the
invariant.
The end effect of this is that if we do not emit any diagnostic, any copy_value
on a move only typed value that is not eliminated by the checker hits an assert
in the verifier... allowing us to know that if a program successfully compiles
that all "move only" proofs successfully ran, guaranteeing safety!
This is an instruction that I am going to use to drive some of the ownership
based dataflow optimizations that I am writing now. The instruction contains a
kind that allows one to know what type of checking is required and allows the
need to add a bunch of independent instructions for independent checkers. Each
checker is responsible for removing all of its own mark instructions. NOTE:
MarkMustCheckInst is only allowed in Raw SIL since once we are in Canonical SIL
we want to ensure that all such checking has already occurred.
NOTE: This is only available when the flag -enable-experimental-move-only. There
are no effects when the flag is disabled.
The way that this works is that it takes advantage of the following changes to
SILGen emission:
* When SILGen initializes a let with NoImplicitCopyAttribute, SILGen now emits
a begin_borrow [lexical] + copy + move_only. This is a pattern that we can check
and know that we are processing a move only value. When performing move
checking, we check move_only as a move only value and that it isn't consumed
multiple times.
* The first point works well for emitting all diagnostics except for
initializing an additional let var. To work around that I changed let
initialization to always bind to an owned value to a move of that owned
value. There is no semantic difference since that value is going to be consumed
by the binding operation anyways so we effectively just move the cleanup from
the original value we wanted to bind to the move. We still then actually borrow
the new let value with a begin_borrow [lexical] for the new let value. This
ensures that an initialization of a let value appears to be a consuming use to
the move only value checker while ensuring that the value has a proper
begin_borrow [lexical].
Some notes on functionality:
1. This attribute can only be applied to local 'let'.
2. "print" due to how we call it today with a vararg array is treated as a
consuming use (unfortunately).
3. I have not added the builtin copy operator yet, but I recently added a _move
skeleton attribute so one can end the lifetimes of these values early.
4. This supports all types that are not address only types (similar to
_move). To support full on address only types we need opaque values.
rdar://83957088
