This fixes a miscompile in case the source of the optimized copy_addr is modified in a called function with to a not visible alias.
This can happen with class properties or global variables.
This fix removes the special handling of function parameters, which was just wrong.
Instead it simply uses the alias analysis API to check for modifications of the source object.
The fix makes TempRValueElimination more conservative and this can cause some performance regressions, but this is unavoidable.
rdar://problem/69605657
* Handle begin_apply in TempRVO
A tempobj passed to begin_apply instruction and cannot be modified by it
(is guaranteed and doesn't alias with other inout args) can be optimzed
away.
We should not optimize away the copy_addr of a guaranteed parameter of a
non-onstack partial_apply.
This is not a bug currently, because TempRVO checks if the lifetime end
points of the temp object are destroy_addr's in
TempRValueOptPass::checkTempObjectDestroy.
This change makes it explicit on which partial_apply's are ok to
optimize.
rdar://61349083
* Use in_guaranteed for let captures
With this all let values will be captured with in_guaranteed convention
by the closure. Following are the main changes :
SILGen changes:
- A new CaptureKind::Immutable is introduced, to capture let values as in_guaranteed.
- SILGen of in_guaranteed capture had to be fixed.
in_guaranteed captures as per convention are consumed by the closure. And so SILGen should not generate a destroy_addr for an in_guaranteed capture.
But LetValueInitialization can push Dealloc and Release states of the captured arg in the Cleanup stack, and there is no way to access the CleanupHandle and disable the emission of destroy_addr while emitting the captures in SILGenFunction::emitCaptures.
So we now create, temporary allocation of the in_guaranteed capture iduring SILGenFunction::emitCaptures without emitting destroy_addr for it.
SILOptimizer changes:
- Handle in_guaranteed in CopyForwarding.
- Adjust dealloc_stack of in_guaranteed capture to occur after destroy_addr for on_stack closures in ClosureLifetimeFixup.
IRGen changes :
- Since HeapLayout can be non-fixed now, make sure emitSize is used conditionally
- Don't consider ClassPointerSource kind parameter type for fulfillments while generating code for partial apply forwarder.
The TypeMetadata of ClassPointSource kind sources are not populated in HeapLayout's NecessaryBindings. If we have a generic parameter on the HeapLayout which can be fulfilled by a ClassPointerSource, its TypeMetaData will not be found while constructing the dtor function of the HeapLayout.
So it is important to skip considering sources of ClassPointerSource kind, so that TypeMetadata of a dependent generic parameters gets populated in HeapLayout's NecessaryBindings.
In the case of copy_addr, we move it onto the source address and in the case of
a store, put it on the source object.
I just noted this pattern happening a bunch in the stdlib when I was looking
through it for ownership patterns.
unchecked_take_enum_data_addr only writes to memory in certain cases. Optional
is not one of those cases luckily.
Given that I am adding support here just for optionals so that we can get rid of
temporaries that are used with a switch_enum_addr. This is an example of a case
where we need to eliminate temporary allocations to allow for Semantic ARC Opts
to eliminate further traffic.
Using fallthroughs in general can lead to easy mistakes. This PR changes the
relevant code to use a helper instead. The reason why is that I am about to add
another instance of needing this exact helper and having to fall through
multiple switch cases will just make this even worse.
Specifically, we do not handle cases where we use projections,
open_existential_addr, or load_borrow. We should be able to handle those in the
future.
The best part is that ossa makes this really easy to do correctly.
rdar://60064817
- consistently operate on the address value of the variable via
stripAccessMarkers. Never use address of the transient access which
may not have sufficient lifetime.
- Recognize begin_access [read] as a load.
- delete dead access markers after deleting copies
Fixes <rdar://59345115> Unnecessary copy_addr not eliminated with
exclusivity checking
There are a few interesting things here:
1. All of this code is conditionalized implicitly on ossa by us checking for
load [take]. If we are in non-ossa, then we will have unqualified loads and this
code path will be skipped. I left in the old test that made sure we did not
support this in non-ossa code for this purpose.
2. We treat the load [take] like a destroy_addr. Thus the current method of
providing safety via using lifetime analysis to show all "destroys" form a
minimal post-dominating set for the copy_addr.
3. We do not allow for load [take] on sub-element initializations. The reason
why is that SILGen initializes temporaries completely in memory and generally
destroys addresses completely as well. Given that is the pattern we are looking
for, we just reduce the state space by banning this pattern.
4. If we have a copy_addr [init], then we not only change the load [take] to
have the original source address as an argument, but we also change the load
[take] to a load [copy] so we don't lose the +1. Additionally, we have to hoist
the load [copy] to the copy_addr [init] site to ensure that we do not insert a
use-after-free from the retain happening too late.
I am doing this to clean up some simple temp rvalue patterns in SIL before
semantic arc opts runs. This ensures that we are more likely to have a load
operation come from a more meaningful semantic entity (for instance a let field
of a guaranteed class) and thus allow us to convert a load [copy] to a
load_borrow.
I am also going to add support in subsequent commits for eliminating alloc_stack
that are stored into as well (another pattern I am seeing).
The pass is already setup for OSSA, so I just enabled it for ownership and
converted its tests over. Eventually, I am going to be able to add support for
eliminating alloc_stack that have a loadabel value and whose lifetime are ended
via a load [take]. But that will be in a forthcoming commit.
