This instruction is similar to a copy_addr except that it marks a move of an
address that has to be checked. In order to keep the memory lifetime verifier
happy, the semantics before the checker runs are the mark_unresolved_move_addr is
equivalent to copy_addr [init] (not copy_addr [take][init]).
The use of this instruction is that Mandatory Inlining converts builtin "move"
to a mark_unresolved_move_addr when inlining the function "_move" (the only
place said builtin is invoked).
This is then run through a special checker (that is later in this PR) that
either proves that the mark_unresolved_move_addr can actually be a move in which
case it converts it to copy_addr [take][init] or if it can not be a move, emit
an error and convert the instruction to a copy_addr [init]. After this is done
for all instructions, we loop back through again and emit an error on any
mark_unresolved_move_addr that were not processed earlier allowing for us to
know that we have completeness.
NOTE: The move kills checker for addresses is going to run after Mandatory
Inlining, but before predictable memory opts and friends.
`ref_to_unowned` and similar instructions must be checked if the operand/result types are treated as "pointers" in escape analysis.
Fixes a miscompile.
https://bugs.swift.org/browse/SR-15527
rdar://85772200
The functions in llvm-project `AttributeList` have been
renamed/refactored to help remove uses of `AttributeList::*Index`.
Update to use these new functions where possible. There's one use of
`AttrIndex` remaining as `replaceAttributeTypeAtIndex` still takes the
index and there is no `param` equivalent. We could add one locally, but
presumably that will be added eventually.
copy_value/destroy_value/begin_borrow were not handled in EscapeAnalysis::analyzeInstruction.
As a result EscapeState of the values were set to Global leading to very conservative results
for queries like CGNode::escapes.
This patch replace all in-memory objects of DebugValueAddrInst with
DebugValueInst + op_deref, and duplicates logics that handles
DebugValueAddrInst with the latter. All related check in the tests
have been updated as well.
Note that this patch neither remove the DebugValueAddrInst class nor
remove `debug_value_addr` syntax in the test inputs.
It was originally convenient for exclusivity optimization to treat
boxes specially. We wanted to know that the 'Box' kind was always
uniquely identified. But that's not really important. And now that
AccessedStorage is being used more generally, the inconsistency is
problematic.
A consistent model is also must easier to understand and explain.
This also make the implementation of the utility simpler and more powerful.
Functional changes:
isRCIdentical will look through mark_dependence and mark_uninitialized.
findReferenceRoot is used consistently everywhere increasing analysis precision.
It's not needed anymore with delayed instruction deletion.
It was used for two purposes:
1. For analysis, which cache instructions, to avoid dangling instruction pointers
2. For passes, which maintain worklists of instructions, to remove a deleted instructions from the worklist. This is now done by checking SILInstruction::isDeleted().
Instead of caching alias results globally for the module, make AliasAnalysis a FunctionAnalysisBase which caches the alias results per function.
Why?
* So far the result caches could only grow. They were reset when they reached a certain size. This was not ideal. Now, they are invalidated whenever the function changes.
* It was not possible to actually invalidate an alias analysis result. This is required, for example in TempRValueOpt and TempLValueOpt (so far it was done manually with invalidateInstruction).
* Type based alias analysis results were also cached for the whole module, while it is actually dependent on the function, because it depends on the function's resilience expansion. This was a potential bug.
I also added a new PassManager API to directly get a function-base analysis:
getAnalysis(SILFunction *f)
The second change of this commit is the removal of the instruction-index indirection for the cache keys. Now the cache keys directly work on instruction pointers instead of instruction indices. This reduces the number of hash table lookups for a cache lookup from 3 to 1.
This indirection was needed to avoid dangling instruction pointers in the cache keys. But this is not needed anymore, because of the new delayed instruction deletion mechanism.
If the "regular" alias analysis thinks that an instruction may write to an address, check if the instruction is in an immutable scope of V.
That means that even if we don't know anything about the instruction (e.g. a call to an unknown function), we can be sure that it cannot write to the address.
An immutable scope is for example a read-only begin_access/end_access scope.
Another example is a borrow scope of an immutable copy-on-write buffer, for example:
%b = begin_borrow %array_buffer
%addr = ref_element_addr [immutable] %b : $BufferType, #BufferType.someField
An error found in DiagnoseInvalidEscapingCaptures can indicate invalid
SIL, which will cause DiagnoseStaticExclusivity to assert during SIL
verification. When the source-level closure captures an inout
argument, it appears in SIL to be a non-escaping closure. The SIL
verification then fails because the "nonescaping" closure actually
escapes.
Ensure that capture diagnostics run on closures before exclusivity
enforcement runs on the parent function. Bypass the SIL verification
assert if a diagnostic error was found.
Fixes rdar://75364904 (Crash with assertion `noescape partial_apply
has unexpected use`)
* add is_escaping_closure as allowed instruction
* don't restrict the uses of copy_value to be a store
Fixes a verification crash (only in assert builds).
https://bugs.swift.org/browse/SR-14394
rdar://75740622
* ``begin_access [modify]`` returned MayWrite, but "modify" means, it can be a read as well.
Instead, return MayReadWrite. Only for ``begin_access [init]`` return MayWrite.
This is more or less cosmetic - probably this bug had no real impact on any optimization.
* ``begin_access [deinit]`` needs to return MayReadWrite for the same reason ``destroy_addr`` returns MayReadWrite (see SILInstruction::MemoryBehavior).
Deinitializer side effects severely handicap the connection-graph
based EscapeAnalysis. Because it's not flow-sensitive, this approach
falls apart whenever an object is released in the current function,
which makes it seem to have escaped everywhere (it generally doesn't
matter if it doesn't escape until the release point, but the analysis
can't discern that).
This can be slightly mitigated by realizing that releasing an object
can only cause things it points to to escape if the object itself has
references or pointers.
Fix: Don't create a escaping content node when releasing an object
that can't contain any references or pointers. The previous commit,
"Fix EscapeAnalysis::mayReleaseContent" would defeat release-hoisting
in important cases without doing anything else. Adding this extra
precision to the connection graph avoids some regressions.
The previous implementation made extremely subtle and specific
assumptions about how the API is used which doesn't apply
everywhere. It was trying very hard to avoid regressing performance
relative to an even older implementation that didn't even try to consider deinitializer side effects.
The aggressive logic was based on the idea that a release must have a
corresponding retain somewhere in the same function and we don't care
if the last release happens early if there are no more aliasing
uses. All the unit tests I wrote previously were based on release
hoisting, which happens to work given the way the API is used.
But this logic is incorrect for retain sinking. In that case sinking
past an "unrelated" release could cause the object to be freed
early. See test/SILOptimizer/arc_crash.swift.
With SemanticARC and other SIL improvements being made, I'm afraid
bugs like this will begin to surface.
To fix it, just remove the subtle logic to leave behind a simple and
sound EscapeAnalysis API. To do better, we will need to rewrite the
AliasAnalysis logic for release side effects, which is currently
a tangled web. In the meantime, SemanticARC can handle many cases without EscapeAnalysis.
Fixes rdar://74469299 (ARC miscompile:
EscapeAnalysis::mayReleaseContent; potential use-after-free)
While fixing this, add support for address-type queries too:
Fixes rdar://74360041 (Assertion failed:
(!releasedReference->getType().isAddress() && "an address is never a
reference"), function mayReleaseContent
strong_retain/strong_release with an optional reference as operand are rejected by the verifier and are not supported by IRGen.
SILCombine created such retains/releases when optimizing ref_cast instructions.
This fixes a compiler crash.
rdar://74146617
If we know that we have a FunctionRefInst (and not another variant of FunctionRefBaseInst), we know that getting the referenced function will not be null (in contrast to FunctionRefBaseInst::getReferencedFunctionOrNull).
NFC
If an argument is deallocated in the function, it cannot be converted to a guaranteed argument.
We already handled the dealloc_ref instruction, but were missing the dealloc_partial_ref instruction.
It fixes a miscompile.
rdar://73871606
Instead make `findJointPostDominatingSet` a stand-alone function.
There is no need to keep the temporary SmallVector alive across multiple calls of findJointPostDominatingSet for the purpose of re-using malloc'ed memory. The worklist usually contains way less elements than its small size.
Some notes:
1. I moved the identity round-trip case to InstSimplify since that is where
optimizations like that are.
2. I did not update in this commit the code that eliminates convert_function
when it is only destroyed. In a subsequent commit I am going to implement
that in a general way and apply it to all forwarding instructions.
3. I implemented eliminating convert_function with ownership only uses in a
utility so that I can reuse it for other similar optimizations in SILCombine.
This removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore.
Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one pointer.
Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore.
Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker).
This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is.
Casting to SILNode cannot be done implicitly, but only with an LLVM `cast` or with SILInstruction::asSILNode(). But this is a rare case anyway.
This removes the ambiguity when casting from a SingleValueInstruction to SILNode, which makes the code simpler. E.g. the "isRepresentativeSILNode" logic is not needed anymore.
Also, it reduces the size of the most used instruction class - SingleValueInstruction - by one pointer.
Conceptually, SILInstruction is still a SILNode. But implementation-wise SILNode is not a base class of SILInstruction anymore.
Only the two sub-classes of SILInstruction - SingleValueInstruction and NonSingleValueInstruction - inherit from SILNode. SingleValueInstruction's SILNode is embedded into a ValueBase and its relative offset in the class is the same as in NonSingleValueInstruction (see SILNodeOffsetChecker).
This makes it possible to cast from a SILInstruction to a SILNode without knowing which SILInstruction sub-class it is.
Casting to SILNode cannot be done implicitly, but only with an LLVM `cast` or with SILInstruction::asSILNode(). But this is a rare case anyway.
Importantly this also lets us use the analysis framework to validate that we do
properly invalidate DeadEndBlocks, preventing bugs.
I did not thread this all over the compiler. Instead I just used it for now in
SemanticARCOpts just to add some coverage without threading it into too many
places.
I am trying to be more careful about this rather than letting someone make this
mistake in the future. I also added some comments and a DeadEndBlocks instance
to TempRValueElimination so that it gets full simplifications in OSSA.
