`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.
Btw for those curious, the reason why this optimization is important is that the
stdlib often times performs reinterpret casts using address to pointer, pointer
to address to perform the case. So we need to be able to reliably eliminate
these. Since the underlying base object's liveness is already what provides the
liveness to the underlying address, our approach of treating this as a lifetime
extension requiring pattern makes sense since we are adding information into the
IR by canonicalizing these escapes to be normal address operations.
NOTE: In this commit, we only handle the identity transform. In a subsequent
commit, I am going to hit the non-identity transform that is handled by
SILCombine.
Also, I added some specific more general OSSA RAUW tests for this case to
ossa_rauw_tests using this functionality in a more exhaustive way that would be
in appropriate in sil_combine_ossa.sil (since I am stress testing this specific
piece of code).
