Merge pull request #34159 from atrick/opt-ref-root

swift-ci · web-flow · commit 971932d0313f · 2020-10-20T19:19:36.000-07:00
diff --git a/include/swift/SIL/MemAccessUtils.h b/include/swift/SIL/MemAccessUtils.h
@@ -1218,6 +1218,10 @@ SILBasicBlock::iterator removeBeginAccess(BeginAccessInst *beginAccess);
 
 namespace swift {
 
+/// Return true if \p svi is a cast that preserves the identity and
+/// reference-counting equivalence of the reference at operand zero.
+bool isRCIdentityPreservingCast(SingleValueInstruction *svi);
+
 /// If \p svi is an access projection, return an address-type operand for the
 /// incoming address.
 ///
diff --git a/lib/SIL/IR/SILType.cpp b/lib/SIL/IR/SILType.cpp
@@ -129,6 +129,9 @@ bool SILType::isPointerSizeAndAligned() {
 //
 // TODO: handle casting to a loadable existential by generating
 // init_existential_ref. Until then, only promote to a heap object dest.
+//
+// This cannot allow trivial-to-reference casts, as required by
+// isRCIdentityPreservingCast.
 bool SILType::canRefCast(SILType operTy, SILType resultTy, SILModule &M) {
   auto fromTy = operTy.unwrapOptionalType();
   auto toTy = resultTy.unwrapOptionalType();
diff --git a/lib/SIL/Utils/InstructionUtils.cpp b/lib/SIL/Utils/InstructionUtils.cpp
@@ -12,6 +12,7 @@
 
 #define DEBUG_TYPE "sil-inst-utils"
 #include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/MemAccessUtils.h"
 #include "swift/AST/SubstitutionMap.h"
 #include "swift/Basic/NullablePtr.h"
 #include "swift/Basic/STLExtras.h"
@@ -64,20 +65,6 @@ SILValue swift::getUnderlyingObjectStopAtMarkDependence(SILValue v) {
   }
 }
 
-static bool isRCIdentityPreservingCast(ValueKind Kind) {
-  switch (Kind) {
-  case ValueKind::UpcastInst:
-  case ValueKind::UncheckedRefCastInst:
-  case ValueKind::UnconditionalCheckedCastInst:
-  case ValueKind::UnconditionalCheckedCastValueInst:
-  case ValueKind::RefToBridgeObjectInst:
-  case ValueKind::BridgeObjectToRefInst:
-    return true;
-  default:
-    return false;
-  }
-}
-
 /// Return the underlying SILValue after stripping off identity SILArguments if
 /// we belong to a BB with one predecessor.
 SILValue swift::stripSinglePredecessorArgs(SILValue V) {
@@ -122,42 +109,39 @@ SILValue swift::stripSinglePredecessorArgs(SILValue V) {
   }
 }
 
-SILValue swift::stripCastsWithoutMarkDependence(SILValue V) {
+SILValue swift::stripCastsWithoutMarkDependence(SILValue v) {
   while (true) {
-    V = stripSinglePredecessorArgs(V);
-
-    auto K = V->getKind();
-    if (isRCIdentityPreservingCast(K)
-        || K == ValueKind::UncheckedTrivialBitCastInst
-        || K == ValueKind::BeginAccessInst
-        || K == ValueKind::EndCOWMutationInst) {
-      V = cast<SingleValueInstruction>(V)->getOperand(0);
-      continue;
+    v = stripSinglePredecessorArgs(v);
+    if (auto *svi = dyn_cast<SingleValueInstruction>(v)) {
+      if (isRCIdentityPreservingCast(svi)
+          || isa<UncheckedTrivialBitCastInst>(v)
+          || isa<BeginAccessInst>(v)
+          || isa<EndCOWMutationInst>(v)) {
+        v = svi->getOperand(0);
+        continue;
+      }
     }
-
-    return V;
+    return v;
   }
 }
 
 SILValue swift::stripCasts(SILValue v) {
   while (true) {
     v = stripSinglePredecessorArgs(v);
-    
-    auto k = v->getKind();
-    if (isRCIdentityPreservingCast(k)
-        || k == ValueKind::UncheckedTrivialBitCastInst
-        || k == ValueKind::MarkDependenceInst
-        || k == ValueKind::BeginAccessInst) {
-      v = cast<SingleValueInstruction>(v)->getOperand(0);
-      continue;
+    if (auto *svi = dyn_cast<SingleValueInstruction>(v)) {
+      if (isRCIdentityPreservingCast(svi)
+          || isa<UncheckedTrivialBitCastInst>(v)
+          || isa<MarkDependenceInst>(v)
+          || isa<BeginAccessInst>(v)) {
+        v = cast<SingleValueInstruction>(v)->getOperand(0);
+        continue;
+      }
     }
-
     SILValue v2 = stripOwnershipInsts(v);
     if (v2 != v) {
       v = v2;
       continue;
     }
-
     return v;
   }
 }
diff --git a/lib/SIL/Utils/MemAccessUtils.cpp b/lib/SIL/Utils/MemAccessUtils.cpp
@@ -365,15 +365,38 @@ bool swift::isLetAddress(SILValue address) {
 //                          MARK: FindReferenceRoot
 //===----------------------------------------------------------------------===//
 
+// On some platforms, casting from a metatype to a reference type dynamically
+// allocates a ref-counted box for the metatype. Naturally that is the place
+// where RC-identity begins. Considering the source of such a casts to be
+// RC-identical would confuse ARC optimization, which might eliminate a retain
+// of such an object completely.
+//
+// The SILVerifier checks that none of these operations cast a nontrivial value
+// to a reference except unconditional_checked_cast[_value].
+bool swift::isRCIdentityPreservingCast(SingleValueInstruction *svi) {
+  switch (svi->getKind()) {
+  default:
+    return false;
+  // Ignore ownership casts
+  case SILInstructionKind::CopyValueInst:
+  case SILInstructionKind::BeginBorrowInst:
+  // Ignore class type casts
+  case SILInstructionKind::UpcastInst:
+  case SILInstructionKind::UncheckedRefCastInst:
+  case SILInstructionKind::RefToBridgeObjectInst:
+  case SILInstructionKind::BridgeObjectToRefInst:
+    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());
+  }
+}
+
 namespace {
 
 // Essentially RC identity where the starting point is already a reference.
-//
-// FIXME: We cannot currently see through type casts for true RC identity,
-// because property indices are not unique within a class hierarchy. Either fix
-// RefElementAddr::getFieldNo so it returns a unique index, or figure out a
-// different way to encode the property's VarDecl. (Note that the lack of a
-// unique property index could be the source of bugs elsewhere).
 class FindReferenceRoot {
   SmallPtrSet<SILPhiArgument *, 4> visitedPhis;
 
@@ -386,18 +409,13 @@ class FindReferenceRoot {
 
 protected:
   // Return an invalid value for a phi with no resolved inputs.
-  //
-  // FIXME: We should be able to see through RC identity like this:
-  //   nextRoot = stripRCIdentityCasts(nextRoot);
   SILValue recursiveFindRoot(SILValue ref) {
-    while (true) {
-      SILValue nextRoot = ref;
-      nextRoot = stripOwnershipInsts(nextRoot);
-      if (nextRoot == ref)
+    while (auto *svi = dyn_cast<SingleValueInstruction>(ref)) {
+      if (!isRCIdentityPreservingCast(svi)) {
         break;
-      ref = nextRoot;
-    }
-
+      }
+      ref = svi->getOperand(0);
+    };
     auto *phi = dyn_cast<SILPhiArgument>(ref);
     if (!phi || !phi->isPhiArgument()) {
       return ref;
@@ -558,8 +576,16 @@ const ValueDecl *AccessedStorage::getDecl(SILValue base) const {
     return global->getDecl();
 
   case Class: {
-    auto *decl = getObject()->getType().getNominalOrBoundGenericNominal();
-    return decl ? getIndexedField(decl, getPropertyIndex()) : nullptr;
+    // The property index is relative to the VarDecl in ref_element_addr, and
+    // can only be reliably determined when the base is avaiable. Otherwise, we
+    // can only make a best effort to extract it from the object type, which
+    // might not even be a class in the case of bridge objects.
+    if (ClassDecl *classDecl =
+            base ? cast<RefElementAddrInst>(base)->getClassDecl()
+                 : getObject()->getType().getClassOrBoundGenericClass()) {
+      return getIndexedField(classDecl, getPropertyIndex());
+    }
+    return nullptr;
   }
   case Tail:
     return nullptr;
@@ -1297,7 +1323,14 @@ void AccessPathDefUseTraversal::followProjection(SingleValueInstruction *svi,
 AccessPathDefUseTraversal::UseKind
 AccessPathDefUseTraversal::visitSingleValueUser(SingleValueInstruction *svi,
                                                 DFSEntry dfs) {
-  if (isAccessedStorageCast(svi)) {
+  if (dfs.isRef()) {
+    if (isRCIdentityPreservingCast(svi)) {
+      pushUsers(svi, dfs);
+      return IgnoredUse;
+    }
+    // 'svi' will be processed below as either RefElementAddrInst,
+    // RefTailAddrInst, or some unknown LeafUse.
+  } else if (isAccessedStorageCast(svi)) {
     pushUsers(svi, dfs);
     return IgnoredUse;
   }
diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp
@@ -3591,6 +3591,15 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     verifyOpenedArchetype(CI, CI->getType().getASTType());
   }
 
+  // Make sure that opcodes handled by isRCIdentityPreservingCast cannot cast
+  // from a trivial to a reference type. Such a cast may dynamically
+  // instantiate a new reference-counted object.
+  void checkNoTrivialToReferenceCast(SingleValueInstruction *svi) {
+    require(!svi->getOperand(0)->getType().isTrivial(*svi->getFunction())
+            || svi->getType().isTrivial(*svi->getFunction()),
+            "Unexpected trivial-to-reference conversion: ");
+  }
+
   /// Verify if a given type is or contains an opened archetype or dynamic self.
   /// If this is the case, verify that the provided instruction has a type
   /// dependent operand for it.
@@ -3753,6 +3762,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   void checkUpcastInst(UpcastInst *UI) {
     require(UI->getType() != UI->getOperand()->getType(),
             "can't upcast to same type");
+    checkNoTrivialToReferenceCast(UI);
     if (UI->getType().is<MetatypeType>()) {
       CanType instTy(UI->getType().castTo<MetatypeType>()->getInstanceType());
       require(UI->getOperand()->getType().is<MetatypeType>(),
diff --git a/test/SILOptimizer/accessed_storage.sil b/test/SILOptimizer/accessed_storage.sil
@@ -131,14 +131,14 @@ struct MyArray<T> {
 
 // CHECK-LABEL: @arrayValue
 // CHECK:   load [trivial] %{{.*}} : $*Builtin.Int64
-// CHECK: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Tail     %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Base:    %{{.*}} = ref_tail_addr [immutable] %{{.*}} : $__ContiguousArrayStorageBase, $Int64
-// CHECK: Storage: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Storage: Tail %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Path:    (@3,#0)
 // CHECK:   load [trivial] %{{.*}} : $*Builtin.Int64
-// CHECK: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Tail    %{{.*}} = struct_extract %5 : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Base:    %{{.*}} = ref_tail_addr [immutable] %{{.*}} : $__ContiguousArrayStorageBase, $Int64
-// CHECK: Storage: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Storage: Tail   %{{.*}} = struct_extract %5 : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Path:    (@4,#0)
 sil [ossa] @arrayValue : $@convention(thin) (@guaranteed MyArray<Int64>) -> Int64 {
 bb0(%0 : @guaranteed $MyArray<Int64>):
@@ -191,7 +191,7 @@ bb0(%0 : $Builtin.RawPointer):
 
 // CHECK-LABEL: @staticIndexAddrSubobject
 // CHECK: ###For MemOp:   %5 = load [trivial] %4 : $*Int64
-// CHECK: Argument %0 = argument of bb0 : $Builtin.RawPointer        // user: %1
+// CHECK: Argument %0 = argument of bb0 : $Builtin.RawPointer
 // CHECK: INVALID
 sil [ossa] @staticIndexAddrSubobject : $@convention(thin) (Builtin.RawPointer) -> () {
 bb0(%0 : $Builtin.RawPointer):
@@ -271,10 +271,10 @@ class B : A {
 // CHECK:   Field: var prop1: Int64 Index: 1
 // CHECK: Path: ()
 // CHECK:   store %0 to %{{.*}} : $*Int64
-// CHECK: Class   %{{.*}} = upcast %{{.*}} : $B to $A
+// CHECK: Class   %{{.*}} = alloc_ref $B
 // CHECK:   Field: var prop0: Int64 Index: 0
 // CHECK: Base:   %{{.*}} = ref_element_addr %{{.*}} : $A, #A.prop0
-// CHECK: Storage: Class   %{{.*}} = upcast %{{.*}} : $B to $A
+// CHECK: Storage: Class   %{{.*}} = alloc_ref $B
 // CHECK:   Field: var prop0: Int64 Index: 0
 // CHECK: Path: ()
 sil @testNonUniquePropertyIndex : $@convention(thin) (Int64) -> () {
@@ -291,21 +291,21 @@ bb0(%0 : $Int64):
 
 // CHECK-LABEL: @testRefTailAndStruct0
 // CHECK: %{{.*}} = load %{{.*}} : $*Builtin.Int64
-// CHECK: Class   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
-// CHECK:   Field: @usableFromInline final var countAndCapacity: _ArrayBody Index: 0
+// CHECK: Class   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
+// CHECK:  Index: 0
 // CHECK: Base:   %{{.*}} = ref_element_addr [immutable] %{{.*}} : $__ContiguousArrayStorageBase, #__ContiguousArrayStorageBase.countAndCapacity
-// CHECK: Storage: Class   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
-// CHECK:   Field: @usableFromInline final var countAndCapacity: _ArrayBody Index: 0
+// CHECK: Storage: Class   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
+// CHECK:  Index: 0
 // CHECK: Path: (#0,#0,#0)
 // CHECK:   %{{.*}} = load %{{.*}} : $*_StringGuts
-// CHECK: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Tail   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Base:   %{{.*}} = ref_tail_addr [immutable] %{{.*}} : $__ContiguousArrayStorageBase, $String
-// CHECK: Storage: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Storage: Tail   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Path: (#0)
 // CHECK: %{{.*}} = load %{{.*}} : $*String
-// CHECK: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Tail   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Base:   %{{.*}} = ref_tail_addr [immutable] %{{.*}} : $__ContiguousArrayStorageBase, $String
-// CHECK: Storage: Tail   %{{.*}} = unchecked_ref_cast %{{.*}} : $Builtin.BridgeObject to $__ContiguousArrayStorageBase
+// CHECK: Storage: Tail   %{{.*}} = struct_extract %{{.*}} : $_MyBridgeStorage, #_MyBridgeStorage.rawValue
 // CHECK: Path: ()
 sil hidden [noinline] @testRefTailAndStruct0 : $@convention(thin) (@owned MyArray<String>) -> () {
 bb0(%0 : $MyArray<String>):
diff --git a/test/SILOptimizer/accessed_storage_analysis.sil b/test/SILOptimizer/accessed_storage_analysis.sil
@@ -638,7 +638,7 @@ class BaseClass {
 class SubClass : BaseClass {}
 
 // CHECK-LABEL: @testElementAddressPhiAccess
-// CHECK: [read] [no_nested_conflict] Class %{{.*}} = upcast %0 : $SubClass to $BaseClass
+// CHECK: [read] [no_nested_conflict] Class %{{.*}} = argument of bb0 : $SubClass
 // CHECK: Field: @_hasInitialValue var f: Float
 sil @testElementAddressPhiAccess : $@convention(thin) (@guaranteed SubClass) -> () {
 bb0(%0 : $SubClass):
diff --git a/test/SILOptimizer/accesspath_uses.sil b/test/SILOptimizer/accesspath_uses.sil
