Merge pull request #42333 from atrick/fix-cast-ownership

atrick · web-flow · commit f35e5e87a228 · 2022-04-13T10:36:50.000-07:00
Fix cast ownership to use the new doesCastPreserveOwnership API
diff --git a/include/swift/SIL/OwnershipUtils.h b/include/swift/SIL/OwnershipUtils.h
@@ -189,9 +189,9 @@ class ForwardingOperand {
     return use->getOwnershipConstraint();
   }
 
-  bool isDirectlyForwarding() const {
+  bool preservesOwnership() const {
     auto &mixin = *OwnershipForwardingMixin::get(use->getUser());
-    return mixin.isDirectlyForwarding();
+    return mixin.preservesOwnership();
   }
 
   ValueOwnershipKind getForwardingOwnershipKind() const;
diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h
@@ -1112,39 +1112,31 @@ class CopyLikeInstruction {
 /// initializes the kind field on our object is run before our constructor runs.
 class OwnershipForwardingMixin {
   ValueOwnershipKind ownershipKind;
-  bool directlyForwards;
+  bool preservesOwnershipFlag;
 
 protected:
   OwnershipForwardingMixin(SILInstructionKind kind,
                            ValueOwnershipKind ownershipKind,
-                           bool isDirectlyForwarding = true)
-      : ownershipKind(ownershipKind), directlyForwards(isDirectlyForwarding) {
+                           bool preservesOwnership = true)
+      : ownershipKind(ownershipKind),
+        preservesOwnershipFlag(preservesOwnership) {
     assert(isa(kind) && "Invalid subclass?!");
     assert(ownershipKind && "invalid forwarding ownership");
-    assert((directlyForwards || ownershipKind != OwnershipKind::Guaranteed) &&
+    assert((preservesOwnershipFlag
+            || ownershipKind != OwnershipKind::Guaranteed) &&
            "Non directly forwarding instructions can not forward guaranteed "
            "ownership");
   }
 
 public:
-  /// If an instruction is directly forwarding, then any operand op whose
-  /// ownership it forwards into a result r must have the property that op and r
-  /// are "rc identical". This means that they are representing the same set of
-  /// underlying lifetimes (plural b/c of aggregates).
+  /// A forwarding instruction preserved ownership if it has a
+  /// dynamically non-trivial result in which all references are forwarded from
+  /// the operand.
   ///
-  /// An instruction that is not directly forwarding, can not have guaranteed
-  /// ownership since without direct forwarding, there isn't necessarily any
-  /// connection in between the operand's lifetime and the value's lifetime.
-  ///
-  /// An example of this is checked_cast_br where when performing the following:
-  ///
-  ///   __SwiftValue(AnyHashable(Klass())) to OtherKlass()
-  ///
-  /// we will look through the __SwiftValue(AnyHashable(X)) any just cast Klass
-  /// to OtherKlass. This means that the result argument would no longer be
-  /// rc-identical to the operand and default case and thus we can not propagate
-  /// forward any form of guaranteed ownership.
-  bool isDirectlyForwarding() const { return directlyForwards; }
+  /// A cast can only forward guaranteed values if it preserves ownership. Such
+  /// casts cannot release any references within their operand's value and
+  /// cannot retain any references owned by their result.
+  bool preservesOwnership() const { return preservesOwnershipFlag; }
 
   /// Forwarding ownership is determined by the forwarding instruction's
   /// constant ownership attribute. If forwarding ownership is owned, then the
@@ -1160,7 +1152,7 @@ class OwnershipForwardingMixin {
     return ownershipKind;
   }
   void setForwardingOwnershipKind(ValueOwnershipKind newKind) {
-    assert((isDirectlyForwarding() || newKind != OwnershipKind::Guaranteed) &&
+    assert((preservesOwnership() || newKind != OwnershipKind::Guaranteed) &&
            "Non directly forwarding instructions can not forward guaranteed "
            "ownership");
     ownershipKind = newKind;
@@ -8083,9 +8075,9 @@ class OwnershipForwardingTermInst : public TermInst,
   OwnershipForwardingTermInst(SILInstructionKind kind,
                               SILDebugLocation debugLoc,
                               ValueOwnershipKind ownershipKind,
-                              bool isDirectlyForwarding = true)
+                              bool preservesOwnership = true)
       : TermInst(kind, debugLoc),
-        OwnershipForwardingMixin(kind, ownershipKind, isDirectlyForwarding) {
+        OwnershipForwardingMixin(kind, ownershipKind, preservesOwnership) {
     assert(classof(kind));
   }
 
@@ -9062,16 +9054,12 @@ class CheckedCastBranchInst final
                         SILBasicBlock *SuccessBB, SILBasicBlock *FailureBB,
                         ProfileCounter Target1Count,
                         ProfileCounter Target2Count,
-                        ValueOwnershipKind forwardingOwnershipKind)
+                        ValueOwnershipKind forwardingOwnershipKind,
+                        bool preservesOwnership)
       : UnaryInstructionWithTypeDependentOperandsBase(
             DebugLoc, Operand, TypeDependentOperands, SuccessBB, FailureBB,
             Target1Count, Target2Count, forwardingOwnershipKind,
-            // We are always directly forwarding unless we are casting an
-            // AnyObject. This is b/c an AnyObject could contain a boxed
-            // AnyObject(Class()) that we unwrap as part of the cast. In such a
-            // case, we would return a different value and potentially end the
-            // lifetime of the operand value.
-            !Operand->getType().isAnyObject()),
+            preservesOwnership),
         DestLoweredTy(DestLoweredTy), DestFormalTy(DestFormalTy),
         IsExact(IsExact) {}
 
diff --git a/lib/SIL/IR/SILInstructions.cpp b/lib/SIL/IR/SILInstructions.cpp
@@ -19,6 +19,7 @@
 #include "swift/Basic/AssertImplements.h"
 #include "swift/Basic/Unicode.h"
 #include "swift/Basic/type_traits.h"
+#include "swift/SIL/DynamicCasts.h"
 #include "swift/SIL/FormalLinkage.h"
 #include "swift/SIL/Projection.h"
 #include "swift/SIL/SILBuilder.h"
@@ -2283,16 +2284,18 @@ CheckedCastBranchInst *CheckedCastBranchInst::create(
     SILBasicBlock *FailureBB, SILFunction &F,
     ProfileCounter Target1Count, ProfileCounter Target2Count,
     ValueOwnershipKind forwardingOwnershipKind) {
-  SILModule &Mod = F.getModule();
+  SILModule &module = F.getModule();
+  bool preservesOwnership = doesCastPreserveOwnershipForTypes(
+    module, Operand->getType().getASTType(), DestFormalTy);
   SmallVector<SILValue, 8> TypeDependentOperands;
   collectTypeDependentOperands(TypeDependentOperands, F, DestFormalTy);
   unsigned size =
       totalSizeToAlloc<swift::Operand>(1 + TypeDependentOperands.size());
-  void *Buffer = Mod.allocateInst(size, alignof(CheckedCastBranchInst));
+  void *Buffer = module.allocateInst(size, alignof(CheckedCastBranchInst));
   return ::new (Buffer) CheckedCastBranchInst(
       DebugLoc, IsExact, Operand, TypeDependentOperands, DestLoweredTy,
       DestFormalTy, SuccessBB, FailureBB, Target1Count, Target2Count,
-      forwardingOwnershipKind);
+      forwardingOwnershipKind, preservesOwnership);
 }
 
 MetatypeInst *MetatypeInst::create(SILDebugLocation Loc, SILType Ty,
diff --git a/lib/SIL/Utils/DynamicCasts.cpp b/lib/SIL/Utils/DynamicCasts.cpp
@@ -320,8 +320,18 @@ bool swift::doesCastPreserveOwnershipForTypes(SILModule &module,
   if (!canIRGenUseScalarCheckedCastInstructions(module, sourceType, targetType))
     return false;
 
-  return !sourceType->isPotentiallyAnyObject()
-    && !targetType->isPotentiallyAnyObject();
+  // (B2) unwrapping
+  if (sourceType->isPotentiallyAnyObject())
+    return false;
+
+  // (B1) wrapping
+  if (targetType->isPotentiallyAnyObject()) {
+    // A class type cannot be wrapped in __SwiftValue, so casting
+    // from a class to AnyObject preserves ownership.
+    return
+      sourceType->mayHaveSuperclass() || sourceType->isClassExistentialType();
+  }
+  return true;
 }
 
 bool SILDynamicCastInst::isRCIdentityPreserving() const {
diff --git a/lib/SIL/Utils/MemAccessUtils.cpp b/lib/SIL/Utils/MemAccessUtils.cpp
@@ -925,7 +925,7 @@ SILValue swift::findOwnershipReferenceAggregate(SILValue ref) {
       if (auto *term = arg->getSingleTerminator()) {
         if (term->isTransformationTerminator()) {
           auto *ti = cast<OwnershipForwardingTermInst>(term);
-          if (ti->isDirectlyForwarding()) {
+          if (ti->preservesOwnership()) {
             root = term->getOperand(0);
             continue;
           }
diff --git a/lib/SIL/Utils/OwnershipUtils.cpp b/lib/SIL/Utils/OwnershipUtils.cpp
@@ -35,19 +35,19 @@ bool swift::canOpcodeForwardGuaranteedValues(SILValue value) {
   if (auto *arg = dyn_cast<SILArgument>(value))
     if (auto *ti = arg->getSingleTerminator())
       if (ti->isTransformationTerminator())
-        return OwnershipForwardingMixin::get(ti)->isDirectlyForwarding();
+        return OwnershipForwardingMixin::get(ti)->preservesOwnership();
 
   if (auto *inst = value->getDefiningInstruction())
     if (auto *mixin = OwnershipForwardingMixin::get(inst))
-      return mixin->isDirectlyForwarding() &&
+      return mixin->preservesOwnership() &&
              !isa<OwnedFirstArgForwardingSingleValueInst>(inst);
 
   return false;
 }
 
 bool swift::canOpcodeForwardGuaranteedValues(Operand *use) {
   if (auto *mixin = OwnershipForwardingMixin::get(use->getUser()))
-    return mixin->isDirectlyForwarding() &&
+    return mixin->preservesOwnership() &&
            !isa<OwnedFirstArgForwardingSingleValueInst>(use->getUser());
   return false;
 }
@@ -58,11 +58,11 @@ bool swift::canOpcodeForwardOwnedValues(SILValue value) {
   if (auto *arg = dyn_cast<SILPhiArgument>(value))
     if (auto *predTerm = arg->getSingleTerminator())
       if (predTerm->isTransformationTerminator())
-        return OwnershipForwardingMixin::get(predTerm)->isDirectlyForwarding();
+        return OwnershipForwardingMixin::get(predTerm)->preservesOwnership();
 
   if (auto *inst = value->getDefiningInstruction())
     if (auto *mixin = OwnershipForwardingMixin::get(inst))
-      return mixin->isDirectlyForwarding() &&
+      return mixin->preservesOwnership() &&
              !isa<GuaranteedFirstArgForwardingSingleValueInst>(inst);
 
   return false;
@@ -71,7 +71,7 @@ bool swift::canOpcodeForwardOwnedValues(SILValue value) {
 bool swift::canOpcodeForwardOwnedValues(Operand *use) {
   auto *user = use->getUser();
   if (auto *mixin = OwnershipForwardingMixin::get(user))
-    return mixin->isDirectlyForwarding() &&
+    return mixin->preservesOwnership() &&
            !isa<GuaranteedFirstArgForwardingSingleValueInst>(user);
   return false;
 }
@@ -1078,7 +1078,7 @@ bool swift::getAllBorrowIntroducingValues(SILValue inputValue,
       auto *arg = cast<SILPhiArgument>(value);
       auto *termInst = arg->getSingleTerminator();
       assert(termInst && termInst->isTransformationTerminator() &&
-             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
+             OwnershipForwardingMixin::get(termInst)->preservesOwnership());
       assert(termInst->getNumOperands() == 1 &&
              "Transforming terminators should always have a single operand");
       worklist.push_back(termInst->getAllOperands()[0].get());
@@ -1130,7 +1130,7 @@ BorrowedValue swift::getSingleBorrowIntroducingValue(SILValue inputValue) {
       auto *arg = cast<SILPhiArgument>(currentValue);
       auto *termInst = arg->getSingleTerminator();
       assert(termInst && termInst->isTransformationTerminator() &&
-             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
+             OwnershipForwardingMixin::get(termInst)->preservesOwnership());
       assert(termInst->getNumOperands() == 1 &&
              "Transformation terminators should only have single operands");
       currentValue = termInst->getAllOperands()[0].get();
@@ -1183,7 +1183,7 @@ bool swift::getAllOwnedValueIntroducers(
       auto *arg = cast<SILPhiArgument>(value);
       auto *termInst = arg->getSingleTerminator();
       assert(termInst && termInst->isTransformationTerminator() &&
-             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
+             OwnershipForwardingMixin::get(termInst)->preservesOwnership());
       assert(termInst->getNumOperands() == 1 &&
              "Transforming terminators should always have a single operand");
       worklist.push_back(termInst->getAllOperands()[0].get());
@@ -1231,7 +1231,7 @@ OwnedValueIntroducer swift::getSingleOwnedValueIntroducer(SILValue inputValue) {
       auto *arg = cast<SILPhiArgument>(currentValue);
       auto *termInst = arg->getSingleTerminator();
       assert(termInst && termInst->isTransformationTerminator() &&
-             OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
+             OwnershipForwardingMixin::get(termInst)->preservesOwnership());
       assert(termInst->getNumOperands()
              - termInst->getNumTypeDependentOperands() == 1 &&
              "Transformation terminators should only have single operands");
diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp
@@ -4139,7 +4139,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                       OwnershipKind::Guaranteed,
               "checked_cast_br with an AnyObject typed source cannot forward "
               "guaranteed ownership");
-      require(CBI->isDirectlyForwarding() ||
+      require(CBI->preservesOwnership() ||
                   CBI->getOperand().getOwnershipKind() !=
                       OwnershipKind::Guaranteed,
               "If checked_cast_br is not directly forwarding, it can not have "
diff --git a/lib/SILGen/SILGenBuilder.cpp b/lib/SILGen/SILGenBuilder.cpp
@@ -18,6 +18,7 @@
 #include "SwitchEnumBuilder.h"
 #include "swift/AST/GenericSignature.h"
 #include "swift/AST/SubstitutionMap.h"
+#include "swift/SIL/DynamicCasts.h"
 
 using namespace swift;
 using namespace Lowering;
@@ -527,10 +528,9 @@ void SILGenBuilder::createCheckedCastBranch(SILLocation loc, bool isExact,
                                             SILBasicBlock *falseBlock,
                                             ProfileCounter Target1Count,
                                             ProfileCounter Target2Count) {
-  // Check if our source type is AnyObject. In such a case, we need to ensure
-  // plus one our operand since SIL does not support guaranteed casts from an
-  // AnyObject.
-  if (op.getType().isAnyObject()) {
+  // Casting a guaranteed value requires ownership preservation.
+  if (!doesCastPreserveOwnershipForTypes(SGF.SGM.M, op.getType().getASTType(),
+                                         destFormalTy)) {
     op = op.ensurePlusOne(SGF, loc);
   }
   createCheckedCastBranch(loc, isExact, op.forward(SGF),
diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyChecker.cpp
@@ -257,7 +257,7 @@ bool CopyOfBorrowedProjectionChecker::check(SILValue markedValue) {
         if (auto op = ForwardingOperand(use)) {
           // If our user is not directly forwarding, we cannot convert its
           // ownership to be guaranteed, so we treat it as a true consuming use.
-          if (!op.isDirectlyForwarding()) {
+          if (!op.preservesOwnership()) {
             foundConsumesOfBorrowedSubValues.push_back(user);
             liveness.updateForUse(user, /*lifetimeEnding*/ true);
             break;
diff --git a/lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp b/lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp
@@ -91,7 +91,7 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
     // here), we could get back a different value. Thus we can not transform
     // such a thing from owned to guaranteed.
     if (auto *i = OwnershipForwardingMixin::get(op->getUser())) {
-      if (!i->isDirectlyForwarding()) {
+      if (!i->preservesOwnership()) {
         tmpUnknownConsumingUses.push_back(op);
         continue;
       }
diff --git a/test/SILGen/opaque_values_silgen.swift b/test/SILGen/opaque_values_silgen.swift
@@ -4,6 +4,8 @@
 
 // FIXME: "HECK" lines all need to be updated for OSSA.
 
+class C {}
+
 func genericInout<T>(_: inout T) {}
 
 func hasVarArg(_ args: Any...) {}
@@ -384,3 +386,50 @@ func testEmptyReturnClosure() {
   let b = nil ?? { bar() }
 }
 
+// Test that PatternMatchEmission::emitIsDispatch can emit a
+// class-to-AnyObject cast as a guaranteed scalar cast
+// (doesCastPreserveOwnershipForTypes returns true).
+//
+// CHECK-LABEL: sil hidden [ossa] @$s20opaque_values_silgen24testCastClassToAnyObjectyyXlAA1CCF : $@convention(thin) (@guaranteed C) -> @owned AnyObject {
+// CHECK: bb0(%0 : @guaranteed $C):
+// CHECK:   checked_cast_br %0 : $C to AnyObject, bb2, bb1
+// CHECK: bb1(%{{.*}} : @guaranteed $C):
+// CHECK: bb2(%{{.*}} : @guaranteed $AnyObject):
+// CHECK-LABEL: } // end sil function
+func testCastClassToAnyObject(_ c: C) -> AnyObject {
+  switch (c) {
+  case let x as AnyObject:
+    _ = x
+    break
+  }
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s20opaque_values_silgen24testCastAnyObjectToClassyAA1CCyXlF : $@convention(thin) (@guaranteed AnyObject) -> @owned C {
+// CHECK: bb0(%0 : @guaranteed $AnyObject):
+// CHECK:   [[CP:%.*]] = copy_value %0 : $AnyObject
+// CHECK:   checked_cast_br [[CP]] : $AnyObject to C, bb1, bb2
+// CHECK-LABEL: } // end sil function '$s20opaque_values_silgen24testCastAnyObjectToClassyAA1CCyXlF'
+func testCastAnyObjectToClass(_ o: AnyObject) -> C {
+  switch (o) {
+  case let x as C:
+    _ = x
+    break
+  default:
+    break
+  }
+}
+
+// CHECK-LABEL: sil hidden [ossa] @$s20opaque_values_silgen024testCastClassArchetypeToF0yAA1CCxRlzClF : $@convention(thin) <T where T : AnyObject> (@guaranteed T) -> @owned C {
+// CHECK: bb0(%0 : @guaranteed $T):
+// CHECK:   [[CP:%.*]] = copy_value %0 : $T
+// CHECK:   checked_cast_br [[CP]] : $T to C, bb1, bb2
+// CHECK-LABEL: } // end sil function '$s20opaque_values_silgen024testCastClassArchetypeToF0yAA1CCxRlzClF'
+func testCastClassArchetypeToClass<T : AnyObject>(_ o: T) -> C {
+  switch (o) {
+  case let x as C:
+    _ = x
+    break
+  default:
+    break
+  }
+}
diff --git a/test/SILOptimizer/rcidentity_opaque.sil b/test/SILOptimizer/rcidentity_opaque.sil

Original file line number	Diff line number	Diff line change
`@@ -189,9 +189,9 @@ class ForwardingOperand {`
`189`	`189`	`return use->getOwnershipConstraint();`
`190`	`190`	`}`
`191`	`191`
`192`		`- bool isDirectlyForwarding() const {`
	`192`	`+ bool preservesOwnership() const {`
`193`	`193`	`auto &mixin = *OwnershipForwardingMixin::get(use->getUser());`
`194`		`- return mixin.isDirectlyForwarding();`
	`194`	`+ return mixin.preservesOwnership();`
`195`	`195`	`}`
`196`	`196`
`197`	`197`	`ValueOwnershipKind getForwardingOwnershipKind() const;`
Original file line number	Diff line number	Diff line change
`@@ -925,7 +925,7 @@ SILValue swift::findOwnershipReferenceAggregate(SILValue ref) {`
`925`	`925`	`if (auto *term = arg->getSingleTerminator()) {`
`926`	`926`	`if (term->isTransformationTerminator()) {`
`927`	`927`	`auto *ti = cast<OwnershipForwardingTermInst>(term);`
`928`		`- if (ti->isDirectlyForwarding()) {`
	`928`	`+ if (ti->preservesOwnership()) {`
`929`	`929`	`root = term->getOperand(0);`
`930`	`930`	`continue;`
`931`	`931`	`}`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)`
`91`	`91`	`// here), we could get back a different value. Thus we can not transform`
`92`	`92`	`// such a thing from owned to guaranteed.`
`93`	`93`	`if (auto *i = OwnershipForwardingMixin::get(op->getUser())) {`
`94`		`- if (!i->isDirectlyForwarding()) {`
	`94`	`+ if (!i->preservesOwnership()) {`
`95`	`95`	`tmpUnknownConsumingUses.push_back(op);`
`96`	`96`	`continue;`
`97`	`97`	`}`