From e8e613bd6ad5b0358f15961627a70d8c1cddc982 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Wed, 21 Oct 2020 22:25:05 +0200 Subject: [PATCH] RCIdentity: fix another case of not-RC-identity-preserving casts. When casting from existentials to class - and vice versa - it can happen that a cast is not RC identity preserving (because of potential bridging). This also affects mayRelease() of such cast instructions. For details see the comments in SILDynamicCastInst::isRCIdentityPreserving(). This change also includes some refactoring: I centralized the logic in SILDynamicCastInst::isRCIdentityPreserving(). rdar://problem/70454804 --- include/swift/SIL/DynamicCasts.h | 3 + lib/SIL/IR/SILInstruction.cpp | 8 +++ lib/SIL/Utils/DynamicCasts.cpp | 44 ++++++++++++ lib/SIL/Utils/MemAccessUtils.cpp | 5 +- .../Analysis/RCIdentityAnalysis.cpp | 49 ++++++------- .../retain_release_code_motion.sil | 68 +++++++++++++++++++ 6 files changed, 145 insertions(+), 32 deletions(-) diff --git a/include/swift/SIL/DynamicCasts.h b/include/swift/SIL/DynamicCasts.h index 712115919f7c9..2f08945b77bc9 100644 --- a/include/swift/SIL/DynamicCasts.h +++ b/include/swift/SIL/DynamicCasts.h @@ -433,6 +433,9 @@ struct SILDynamicCastInst { return TargetIsBridgeable != SourceIsBridgeable; } + /// Returns true if this dynamic cast can release its source operand. + bool isRCIdentityPreserving() const; + /// If getSourceType() is a Swift type that can bridge to an ObjC type, return /// the ObjC type it bridges to. If the source type is an objc type, an empty /// CanType() is returned. diff --git a/lib/SIL/IR/SILInstruction.cpp b/lib/SIL/IR/SILInstruction.cpp index 9b602c7d4ee5f..7d05a902c11b8 100644 --- a/lib/SIL/IR/SILInstruction.cpp +++ b/lib/SIL/IR/SILInstruction.cpp @@ -22,6 +22,7 @@ #include "swift/SIL/SILCloner.h" #include "swift/SIL/SILDebugScope.h" #include "swift/SIL/SILVisitor.h" +#include "swift/SIL/DynamicCasts.h" #include "swift/Basic/AssertImplements.h" #include "swift/ClangImporter/ClangModule.h" #include "swift/SIL/SILModule.h" @@ -1059,6 +1060,13 @@ bool SILInstruction::mayHaveSideEffects() const { } bool SILInstruction::mayRelease() const { + // Overrule a "DoesNotRelease" of dynamic casts. If a dynamic cast is not + // RC identity preserving it can release it's source (in some cases - we are + // conservative here). + auto dynCast = SILDynamicCastInst::getAs(const_cast(this)); + if (dynCast && !dynCast.isRCIdentityPreserving()) + return true; + if (getReleasingBehavior() == SILInstruction::ReleasingBehavior::DoesNotRelease) return false; diff --git a/lib/SIL/Utils/DynamicCasts.cpp b/lib/SIL/Utils/DynamicCasts.cpp index ffbd852e73ab5..5c7998bec7856 100644 --- a/lib/SIL/Utils/DynamicCasts.cpp +++ b/lib/SIL/Utils/DynamicCasts.cpp @@ -208,6 +208,50 @@ static CanType getHashableExistentialType(ModuleDecl *M) { return hashable->getDeclaredInterfaceType()->getCanonicalType(); } +static bool canBeExistential(CanType ty) { + // If ty is an archetype, conservatively assume it's an existential. + return ty.isAnyExistentialType() || ty->is(); +} + +static bool canBeClass(CanType ty) { + // If ty is an archetype, conservatively assume it's an existential. + return ty.getClassOrBoundGenericClass() || ty->is(); +} + +bool SILDynamicCastInst::isRCIdentityPreserving() const { + // Casts which cast from a trivial type, like a metatype, to something which + // is retainable (or vice versa), like an AnyObject, are not RC identity + // preserving. + // On some platforms such casts dynamically allocate a ref-counted box for the + // metatype. Naturally that is the place where a new rc-identity begins. + // Therefore such a cast is introducing a new rc identical object. + // + // If RCIdentityAnalysis would look through such a cast, ARC optimizations + // would get confused and might eliminate a retain of such an object + // completely. + SILFunction &f = *getFunction(); + if (getSourceLoweredType().isTrivial(f) != getTargetLoweredType().isTrivial(f)) + return false; + + CanType source = getSourceFormalType(); + CanType target = getTargetFormalType(); + + // An existential may be holding a reference to a bridgeable struct. + // In this case, ARC on the existential affects the refcount of the container + // holding the struct, not the class to which the struct is bridged. + // Therefore, don't assume RC identity when casting between existentials and + // classes (and also between two existentials). + if (canBeExistential(source) && + (canBeClass(target) || canBeExistential(target))) + return false; + + // And vice versa. + if (canBeClass(source) && canBeExistential(target)) + return false; + + return true; +} + /// Check if a given type conforms to _BridgedToObjectiveC protocol. bool swift::isObjectiveCBridgeable(ModuleDecl *M, CanType Ty) { // Retrieve the _BridgedToObjectiveC protocol. diff --git a/lib/SIL/Utils/MemAccessUtils.cpp b/lib/SIL/Utils/MemAccessUtils.cpp index 922fb78ea8894..73d9d3e43920d 100644 --- a/lib/SIL/Utils/MemAccessUtils.cpp +++ b/lib/SIL/Utils/MemAccessUtils.cpp @@ -15,6 +15,7 @@ #include "swift/SIL/MemAccessUtils.h" #include "swift/SIL/SILModule.h" #include "swift/SIL/SILUndef.h" +#include "swift/SIL/DynamicCasts.h" #include "llvm/Support/Debug.h" using namespace swift; @@ -388,9 +389,7 @@ bool swift::isRCIdentityPreservingCast(SingleValueInstruction *svi) { return true; case SILInstructionKind::UnconditionalCheckedCastInst: case SILInstructionKind::UnconditionalCheckedCastValueInst: - // If the source is nontrivial, then this checked cast may actually create a - // new object, so its source is not ref-count equivalent. - return !svi->getOperand(0)->getType().isTrivial(*svi->getFunction()); + return SILDynamicCastInst(svi).isRCIdentityPreserving(); } } diff --git a/lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp b/lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp index a41a67e027ebd..45394c1376aa7 100644 --- a/lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp @@ -13,6 +13,7 @@ #include "swift/SILOptimizer/Analysis/RCIdentityAnalysis.h" #include "swift/SILOptimizer/Analysis/DominanceAnalysis.h" #include "swift/SIL/SILInstruction.h" +#include "swift/SIL/DynamicCasts.h" #include "llvm/Support/CommandLine.h" using namespace swift; @@ -34,21 +35,6 @@ static bool isNoPayloadEnum(SILValue V) { return !EI->hasOperand(); } -/// Returns true if V is a valid operand of a cast which preserves RC identity. -/// -/// V is a valid operand if it has a non-trivial type. -/// RCIdentityAnalysis must not look through casts which cast from a trivial -/// type, like a metatype, to something which is retainable, like an AnyObject. -/// On some platforms such casts dynamically allocate a ref-counted box for the -/// metatype. Naturally that is the place where a new rc-identity begins. -/// Therefore such a cast is introducing a new rc identical object. -/// -/// If we would look through such a cast, ARC optimizations would get confused -/// and might eliminate a retain of such an object completely. -static bool isValidRCIdentityCastOperand(SILValue V, SILFunction *F) { - return !V->getType().isTrivial(*F); -} - /// RC identity is more than a guarantee that references refer to the same /// object. It also means that reference counting operations on those references /// have the same semantics. If the types on either side of a cast do not have @@ -61,16 +47,16 @@ static SILValue getRCIdentityPreservingCastOperand(SILValue V) { switch (V->getKind()) { case ValueKind::UpcastInst: case ValueKind::UncheckedRefCastInst: - case ValueKind::UnconditionalCheckedCastInst: case ValueKind::InitExistentialRefInst: case ValueKind::OpenExistentialRefInst: case ValueKind::RefToBridgeObjectInst: case ValueKind::BridgeObjectToRefInst: - case ValueKind::ConvertFunctionInst: { - auto *inst = cast(V); - SILValue castOp = inst->getOperand(0); - if (isValidRCIdentityCastOperand(castOp, inst->getFunction())) - return castOp; + case ValueKind::ConvertFunctionInst: + return cast(V)->getOperand(0); + case ValueKind::UnconditionalCheckedCastInst: { + auto *castInst = cast(V); + if (SILDynamicCastInst(castInst).isRCIdentityPreserving()) + return castInst->getOperand(); break; } default: @@ -141,8 +127,10 @@ static SILValue stripRCIdentityPreservingInsts(SILValue V) { if (SILValue Result = A->getSingleTerminatorOperand()) { // In case the terminator is a conditional cast, Result is the source of // the cast. - if (isValidRCIdentityCastOperand(Result, A->getFunction())) - return Result; + auto dynCast = SILDynamicCastInst::getAs(A->getSingleTerminator()); + if (dynCast && !dynCast.isRCIdentityPreserving()) + return SILValue(); + return Result; } } @@ -347,12 +335,15 @@ SILValue RCIdentityFunctionInfo::stripRCIdentityPreservingArgs(SILValue V, } unsigned IVListSize = IncomingValues.size(); - if (IVListSize == 1 && - !isValidRCIdentityCastOperand(IncomingValues[0].second, A->getFunction())) - return SILValue(); - - assert(IVListSize != 1 && "Should have been handled in " - "stripRCIdentityPreservingInsts"); + if (IVListSize == 1) { +#ifndef NDEBUG + auto dynCast = SILDynamicCastInst::getAs(A->getSingleTerminator()); + assert((dynCast && !dynCast.isRCIdentityPreserving()) + && "Should have been handled in stripRCIdentityPreservingInsts"); +#endif + return SILValue(); + + } // Ok, we have multiple predecessors. See if all of them are the same // value. If so, just return that value. diff --git a/test/SILOptimizer/retain_release_code_motion.sil b/test/SILOptimizer/retain_release_code_motion.sil index a2efe718a07a2..cb4c7d7c00eb4 100644 --- a/test/SILOptimizer/retain_release_code_motion.sil +++ b/test/SILOptimizer/retain_release_code_motion.sil @@ -31,6 +31,8 @@ struct A { class fuzz { } +protocol P : class { } + enum Boo { case one case two @@ -817,3 +819,69 @@ bb0(%0 : $_ContiguousArrayBuffer, %1 : $Builtin.Word, %2 : $Builtin.W release_value %0 : $_ContiguousArrayBuffer return %newptr : $Builtin.RawPointer } + +// CHECK-LABEL: sil @dontMoveOverExistentialToClassCast : $@convention(thin) (@guaranteed AnyObject) -> Optional +// CHECK: strong_retain %0 +// CHECK: checked_cast_br %0 +// CHECK: } // end sil function 'dontMoveOverExistentialToClassCast' +sil @dontMoveOverExistentialToClassCast : $@convention(thin) (@guaranteed AnyObject) -> Optional { +bb0(%0 : $AnyObject): + strong_retain %0 : $AnyObject + checked_cast_br %0 : $AnyObject to fuzz, bb1, bb2 + +bb1(%18 : $fuzz): + %19 = enum $Optional, #Optional.some!enumelt, %18 : $fuzz + br bb3(%19 : $Optional) + +bb2: + strong_release %0 : $AnyObject + %22 = enum $Optional, #Optional.none!enumelt + br bb3(%22 : $Optional) + +bb3(%24 : $Optional): + return %24 : $Optional +} + +// CHECK-LABEL: sil @dontMoveOverClassToExistentialCast : $@convention(thin) (@guaranteed fuzz) -> Optional

+// CHECK: strong_retain %0 +// CHECK: checked_cast_br %0 +// CHECK: } // end sil function 'dontMoveOverClassToExistentialCast' +sil @dontMoveOverClassToExistentialCast : $@convention(thin) (@guaranteed fuzz) -> Optional

{ +bb0(%0 : $fuzz): + strong_retain %0 : $fuzz + checked_cast_br %0 : $fuzz to P, bb1, bb2 + +bb1(%18 : $P): + %19 = enum $Optional

, #Optional.some!enumelt, %18 : $P + br bb3(%19 : $Optional

) + +bb2: + strong_release %0 : $fuzz + %22 = enum $Optional

, #Optional.none!enumelt + br bb3(%22 : $Optional

) + +bb3(%24 : $Optional

): + return %24 : $Optional

+} + +// CHECK-LABEL: sil @dontMoveOverExistentialToExistentialCast : $@convention(thin) (@guaranteed AnyObject) -> Optional

+// CHECK: strong_retain %0 +// CHECK: checked_cast_br %0 +// CHECK: } // end sil function 'dontMoveOverExistentialToExistentialCast' +sil @dontMoveOverExistentialToExistentialCast : $@convention(thin) (@guaranteed AnyObject) -> Optional

{ +bb0(%0 : $AnyObject): + strong_retain %0 : $AnyObject + checked_cast_br %0 : $AnyObject to P, bb1, bb2 + +bb1(%18 : $P): + %19 = enum $Optional

, #Optional.some!enumelt, %18 : $P + br bb3(%19 : $Optional

) + +bb2: + strong_release %0 : $AnyObject + %22 = enum $Optional

, #Optional.none!enumelt + br bb3(%22 : $Optional

) + +bb3(%24 : $Optional

): + return %24 : $Optional

+}