While looking at the performance of the verifier running with -sil-verify-all on
the stdlib, I noticed that we are spending ~30% of the total time in the
verifier performing this check. Introducing the cache mitigates this issue.
I believe the reason is that we were walking for each operand the use list of
its associated value which I think is quadratic.
In OSSA, there is no reason for the cond_br instruction to take arguments.
In OSSA utilities, we have two types of block arguments: terminator
results and phis. cond_br arguments don't fit into either one of these
categories. Supporting them would be prohibitively complex. In fact, I
have no idea model it.
I have already made the assumption in several places that we don't
support cond_br args. It would be impossible to test all the code that
makes this assumption, so even if we wanted to support cond_br args,
they would always be broken in practice.
Simply mark them illegal in the SILVerifier and make sure no OSSA
passes generate them.
Compiler:
- Add `Forward` and `Reverse` to `DifferentiabilityKind`.
- Expand `DifferentiabilityMask` in `ExtInfo` to 3 bits so that it now holds all 4 cases of `DifferentiabilityKind`.
- Parse `@differentiable(reverse)` and `@differentiable(_forward)` declaration attributes and type attributes.
- Emit a warning for `@differentiable` without `reverse`.
- Emit an error for `@differentiable(_forward)`.
- Rename `@differentiable(linear)` to `@differentiable(_linear)`.
- Make `@differentiable(reverse)` type lowering go through today's `@differentiable` code path. We will specialize it to reverse-mode in a follow-up patch.
ABI:
- Add `Forward` and `Reverse` to `FunctionMetadataDifferentiabilityKind`.
- Extend `TargetFunctionTypeFlags` by 1 bit to store the highest bit of differentiability kind (linear). Note that there is a 2-bit gap in `DifferentiabilityMask` which is reserved for `AsyncMask` and `ConcurrentMask`; `AsyncMask` is ABI-stable so we cannot change that.
_Differentiation module:
- Replace all occurrences of `@differentiable` with `@differentiable(reverse)`.
- Delete `_transpose(of:)`.
Resolves rdar://69980056.
I also added a SILVerifier check that once we are in canonical SIL all
concurrent functions that are partial applied are banned from having Box
arguments.
I have not added support yet for this for address only types, so we do not crash
on that yet.
TLDR: This is just an NFC rename in preparation for changing
SILValue::getOwnershipKind() of any forwarding instructions to return
OwnershipKind::None if they have a trivial result despite forwarding ownership
that isn't OwnershipKind::None (consider an unchecked_enum_data of a trivial
payload from a non-trivial enum).
This ensures that one does not by mistake use this routine instead of
SILValue::getOwnershipKind(). The reason why these two things must be
distinguished is that the forwarding ownership kind of an instruction that
inherits from OwnershipForwardingMixin is explicitly not the ValueOwnershipKind
of the result of the instruction. Instead it is a separate piece of state that:
1. For certain forwarding instructions, defines the OwnershipConstraint of the
forwarding instruction.
2. Defines the ownership kind of the result of the value. If the result of the
value is non-trivial then it is exactly the set ownership kind. If the result is
trivial, we use OwnershipKind::None instead. As an example of this, consider an
unchecked_enum_data that extracts from a non-trivial enum a trivial payload:
```
enum Either {
case int(Int)
case obj(Klass)
}
%1 = load_borrow %0 : $*Either
%2 = unchecked_enum_data %1 : $Either, #Either.int!enumelt.1 // Int type
end_borrow %1 : $Either
```
If we were to identify the forwarding ownership kind (guaranteed) of
unchecked_enum_data with the value ownership kind of its result, we would
violate ownership since we would be passing a guaranteed value to the operand of
the unchecked_enum_data that will only accept values with
OwnershipKind::None. =><=.
This visits unique pairs of reborrowPhi and its base value.
The code for the utility is mostly refactored from the ReborrowVerifier.
This utility will be used later in DCE.
...for handling borrow scopes:
- find[Extended]TransitiveGuaranteedUses
- BorrowingOperand::visit[Extended]ScopeEndingUses
- BorrowedValue BorrowingOperand::getBorrowIntroducingUserResult()
And document the logic.
Mostly NFC in this commit, but more RAUW cases should be correctly
handled now.
Particularly, ensure that we can cleanly handle all manner of
reborrows. This is necessary to ensure both CanonicalizeOSSA and
replace-all-uses higher-level utilities handle all cases.
This generalizes some of the logic in CanonicalizeOSSA so it can be
shared by other high-level OSSA utilities.
These utilities extend the fundamental BorrowingOperand and
BorrowedValue functionality that has been designed recently. It builds
on and replaces a mix of piece-meal functionality that was needed
during bootstrapping. These APIs are now consistent with the more
recently designed code. It should be obvious what they mean and how to
use them, should be very hard to use them incorrectly, and should be
as efficient as possible, since they're fundamental to other
higher-level utilities.
Most importantly, there were several very subtle correctness issues
that were not handled cleanly in a centralized way. There are now a
mix of higher-level utilities trying to use this underlying
functionality, but it was hard to tell if all those higher-level
utilities were covering all the subtle cases:
- checking for PointerEscapes everywhere we need to
- the differences between uses of borrow introducers and other
guaranteed values
- handling the uses of nested borrow scopes
- transitively handling reborrows
- the difference between nested and top-level reborrows
- forwarding destructures (which can cause exponential explosion)
In short, I was fundamentally confused and getting things wrong before
designing these utilities.
Currently in this verifier, we stop verifying if we find a switch_enum_addr
use. This creates a problem since no one has gone through and changed the
frontend/optimizer to understand that it needs to insert destroy_addr on
alloc_stack even if dynamically we know that the enum has a trivial case or
non-payloaded case due to a switch_enum_addr. So if one then performs the
completely unrelated, valid optimization of promoting the switch_enum_addr to a
switch_enum (lets say using a load_borrow), then this verifier will see a leaked
value along the non-payloaded path.
Disable this verification for now until a complete analysis of enums is provided
to unblock further work.
https://bugs.swift.org/browse/SR-14123
It used to be that we had certain instructions that could only have operands
with OwnershipKind::{None,Unowned}. This changed with the addition of
OperandOwnership which caused us to begin accepting owned and guaranteed in all
positions where we accept unowned values with the proviso that certain
forwarding instructions are now allowed to force their result to be unowned even
though their argument is owned or guaranteed.
This setup massively simplifies RAUWing values in OSSA by eliminating the need
to insert conversion unchecked_ownership_conversion instructions from
owned->unowned when replacing an unowned value with an owned or guaranteed
value. Now, these uses can just take the owned or guaranteed value directly and
we wish to maintain that property. Thus we are banning it in SIL by writing a
verifier check.
Instead, I just added some static helper methods that perform the same
operations without needing to deal with generics/etc on OwnershipForwardingMixin
itself. The reason why I did this is that this Mixin is not part of the SILNode
heirarchy so we shouldn't use utilities tied to the SILNode hierarchy.
Migrating to this classification was made easy by the recent rewrite
of the OSSA constraint model. It's also consistent with
instruction-level abstractions for working with different kinds of
OperandOwnership that are being designed.
This classification vastly simplifies OSSA passes that rewrite OSSA
live ranges, making it straightforward to reason about completeness
and correctness. It will allow a simple utility to canonicalize OSSA
live ranges on-the-fly.
This avoids the need for OSSA-based utilities and passes to hard-code
SIL opcodes. This will allow several of those unmaintainable pieces of
code to be replaced with a trivial OperandOwnership check.
It's extremely important for SIL maintainers to see a list of all SIL
opcodes associated with a simple OSSA classification and set of
well-specified rules for each opcode class, without needing to guess
or reverse-engineer the meaning from the implementation. This
classification does that while eliminating a pile of unreadable
macros.
This classification system is the model that CopyPropagation was
initially designed to use. Now, rather than relying on a separate
pass, a simple, lightweight utility will canonicalize OSSA
live ranges.
The major problem with writing optimizations based on OperandOwnership
is that some operations don't follow structural OSSA requirements,
such as project_box and unchecked_ownership_conversion. Those are
classified as PointerEscape which prevents the compiler from reasoning
about, or rewriting the OSSA live range.
Functional Changes:
As a side effect, this corrects many operand constraints that should
in fact require trivial operand values.
It would be more abstractly correct if this got DI support so
that we destroy the member if the constructor terminates
abnormally, but we can get to that later.
The SIL type of the keypath instruction is lowered in the function's
type expansion context, the various types involved in patterns are
unlowered AST types.
When verifying that the types matchup apply the type expansion to both
sides.
rdar://72134937
This commit is doing a few things:
1. It is centralizing all decisions about whether an operand's owner instruction
or a value's parent instruction is forwarding in each SILInstruction
itself. This will prevent this information from getting out of sync.
2. This allowed me to hide the low level queries in OwnershipUtils.h that
determined if a SILNodeKind was "forwarding". I tried to minimize the amount of
churn in this PR and thus didn't remove the
is{Owned,Ownership,Guaranteed}Forwarding{Use,Value} checks. Instead I left them
alone but added in asserts to make sure that if the old impl ever returns true,
the neew impl does as well. In a subsequent commit, I am going to remove the old
impl in favor of isa queries.
3. I also in the process discovered that there were some instructions that were
being inconsistently marked as forwarding. All of the asserts in the PR caught
these and I fixed these inconsistencies.
This originally trafficked in Instructions and for some reason the name was
never changed.
I also changed the result type to be a bool and added the ability for the passed
in closure to signal failure (and iteration stop) by returning false. This also
makes it possible to use visitLocalEndScopeUses in if statements which can be
useful.
This condition does not hold after each pass. It's only the case after running some cleanup-passes.
This verifier condition makes it impossible to bisect the optimization passes.
I think what was happening here was that we were using one of the superclass
classofs and were getting lucky since in the place I was using this I was
guaranteed to have single value instructions and that is what I wrote as my
first case X ).
I also added more robust checks tieing the older isGuaranteed...* APIs to the
ForwardingOperand API. I also eliminated the notion of Branch being an owned
forwarding instruction. We only used this in one place in the compiler (when
finding owned value introducers), yet we treat a phi as an introducer, so we
would never hit a branch in our search since we would stop at the phi argument.
The bigger picture here is that this means that all "forwarding instructions"
either forward ownership for everything or for everything but owned/unowned.
And for those listening in, I did find one instruction that was from an
ownership forwarding subclass but was not marked as forwarding:
DifferentiableFunctionInst. With this change, we can no longer by mistake have
such errors enter the code base.
This assert does not make sense because consumingUses in some cases
only contains the destroying uses. Owned values may not be destroyed
because they may be converted to ValueOwnershipKind::None on all paths
reaching a return. Instead, this utility needs to find liveness first
considering all uses (or at least all uses that may be on a lifetime
boundary). We probably then won't need this assert, but I'm leaving
the FIXME as a placeholder for that work.
Fixes rdar://71240363.
