Copy propagation does these optimizations more generally. It should be
able to replace this optimization. Fixing it and not just deleting CopyValueOpts
because it happens to be called in the mandatory pipeline.
Prevent joining lifetimes of copies with destroyed owned lexical values
if there may be deinit barriers between the final consume of the copy
and the destroy of the lexical value.
rdar://108014714
Fix a miscompile in Debug builds at -Onone.
This optimization ignores uses of owned values that aren't enclosed in
borrow scopes. This is fairly eggregious since project_box
instructions are never borrowed, which means that all local variables
have this problem.
This is a well-known issue that occurs throughout OSSA
optimizations. The reason that we don't see the problem often is that
the optimizations are hidden behind a pile of ad-hoc pattern matching,
so they only kick in for simple cases. This approach to optimization
is great at hiding problems for a long time.
We're attempting to design away this class of problems in the next
release. Until then, it's one miscompile at a time.
Fixes rdar://107420448 (Variable mutation in block isn't reflected
in outer scope: new behavior in swift 5.9)
In the absence of this bailout, reborrows can be introduced while replacing the @owned value with
a local borrow introducer. Since lifetime adjustment for this case was not implemented, disable here.
rdar://106757446
Start using consistent terminolfy in ownership utils.
A transitive use set follows transitive uses within an ownership
lifetime. It does not rely on complete inner scopes. An extended use
set is not necessarilly transitive but does look across
lifetime-ending uses: copies of owned values and/or reborrows of
guaranteed values. Whether lifetime extension refers to copies or
reborrow is context dependent.
Andy some time ago already created the new API but didn't go through and update
the old occurences. I did that in this PR and then deprecated the old API. The
tree is clean, so I could just remove it, but I decided to be nicer to
downstream people by deprecating it first.
SemanticARCOptVisitor::performGuaranteedCopyValueOptimization was
converting this SIL
%borrow = begin_borrow %copiedValue
%copy = copy_value %borrow
%borrowCopy = begin_borrow %copy
end_borrow %borrow
end_borrow %borrowCopy
destroy_value %copy
// something something
unreachable
into
%borrow = begin_borrow %copiedValue
%innerBorrow = begin_borrow %borrow
end_borrow %borrow
end_borrow %innerBorrow
// something something
unreachable
Dead-end blocks are simply irrelevant for this
optimization. Unfortunately, there were multiple layers of attempted
workarounds that were hiding the real problem, except in rare cases.
Thanks Nate Chandler for reducing the test.
This abstraction is heavily used to ask about the current borrow scope
enclosing some uses. Give it the functionality it needs.
Add BorrowedValue::computeLiveness(PrunedLiveness). Get the borrow
scope's live range. A trivial 4-line implementation.
Cleanup BorrowedValue::areUsesWithinScope(). Quick check to see if a set
of uses known to be dominated by the borrow are within the borrow scope.
Add BorrowedValue::hasReborrow(). Is this local borrow scope
reborrowed? If not, then it's local scope-ending operations are the
end of its required lifetime.
Simplify BorrowedValue::gatherReborrows() to use OperandOwnership::Reborrow.
Don't allow an owned call argument to be considered a valid BorrowingOperand.
More generally, make sure there is a perfect equivalence between valid
BorrowingOperand and the corresponding OperandOwnership kind.
This bleeds into the implementation where "guaranteed" is used
everywhere to talk about optimization of guaranteed values. We need to
use mandatory to indicate we're talking about the pass pipeline.
...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.
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.
The specific problem here is that I am going to be adding some code to
SemanticARCOpts that eliminates reborrows and may need to create new phi
arguments and thus add arguments to edges. The weird thing about this is that
doing so actually requires us to create a new terminator!
This means that subtle pointer invalidation issues can occur here. To work
around that we store our terminators as SILBasicBlock, operand number since we
can always immediately find a terminator from its basic block. If we do not have
a terminator, we keep on just storing the SILInstruction itself.
NOTE: This only saves us from additive changes. Deletions are still an issue.
Specifically the optimization that is being performed here is the elimination of
lifetimes that hand off ownership in between two regions of code. Example:
```
%1 = copy_value %0
...
destroy_value %0
...
apply %consumingUser(%1)
```
We really want to handle this case since it is a natural thing that comes up in
programs and will let me eliminate the *evil* usage of emitDestroyValueOperation
in TypeLowering without needing to modify huge amounts of tests.
This split will ensure we are testing only what a specific optimization is doing
rather than all together.
NOTE: The way I split up the tests is I first split up each of the tests by
subject area by hand that I thought were specifically testing one pass. Then any
tests where the FileCheck tests started to fail due to us not running the other
passes, I put back a copy in the original semantic-arc-opts.sil to ensure we do
not regress.
Note, the tests that I copied for each of these passes
are originally from semantic-arc-opts.sil. I left them there as well so we could
see all of the opts together. I also only copied the ones that were testing pass
specific functionality.
This doesn't happen as much as the guaranteed + owned value optimization but can
occur once we start eliminating borrow scopes. I noticed that I needed this when
looking at the arbitrary RAUW for ownership API.
When I originally landed this optimization, I was trying to handle simple cases
without forwarding instructions in my head. In my head, handling this with
forwarding instructions meant using OwnershipLiveRange. Sadly, I didn't realize
at the time that if I didn't use OwnershipLiveRange and just treated forwarding
insts as normal lifetime ending instructions for owned values, everything just
worked (since our check was where %5 in the following was any lifetime ending
instruction for %4c.
```
%4 = copy_value %3 : $Builtin.NativeObject
%4c = copy_value %4 : $Builtin.NativeObject
destroy_value %4 : $Builtin.NativeObject
%5 = enum $FakeOptional<Builtin.NativeObject>, #FakeOptional.some!enumelt, %4c : $Builtin.NativeObject
```
The optimization causes the above to simplified to:
```
%4 = copy_value %3 : $Builtin.NativeObject
%5 = enum $FakeOptional<Builtin.NativeObject>, #FakeOptional.some!enumelt, %4 : $Builtin.NativeObject
```
Notice importantly that we are not shrinking the lifetime of %4, so we can do
this safely without interior pointers being guarded with borrow scopes
everywhere yet.
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 makes it easier to understand conceptually why a ValueOwnershipKind with
Any ownership is invalid and also allowed me to explicitly document the lattice
that relates ownership constraints/value ownership kinds.
This patch moves state from the main SemanticARCOptVisitor struct to instead be
on a context object. Sub-transformations should not need to know about the
visitor since how it processes things is orthogonal from the transformations
themselves.
