Add EscapeAnalysis::findObjectKind to mitigate deinit side effects

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.

Author: atrick

include/swift/SILOptimizer/Analysis/EscapeAnalysis.h

@@ -989,8 +989,13 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
   /// The allocator for the connection graphs in Function2ConGraph.
   llvm::SpecificBumpPtrAllocator<FunctionInfo> Allocator;
 
+  // TODO: Use per-function caches, at least for Resilient types, to use Maximal
+  // expansion.
+
   /// Cache for isPointer().
   PointerKindCache pointerKindCache;
+  /// Cache for checking the aggregate pointerness of class properties.
+  PointerKindCache classPropertiesKindCache;
 
   SILModule *M;
 
@@ -1014,6 +1019,12 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
 
   PointerKind findCachedPointerKind(SILType Ty, const SILFunction &F) const;
 
+  PointerKind findClassPropertiesPointerKind(SILType Ty,
+                                             const SILFunction &F) const;
+
+  PointerKind findCachedClassPropertiesKind(SILType Ty,
+                                            const SILFunction &F) const;
+
   // Returns true if the type \p Ty must be a reference or must transitively
   // contain a reference and no other pointer or address type.
   bool hasReferenceOnly(SILType Ty, const SILFunction &F) const {

lib/SILOptimizer/Analysis/AliasAnalysis.cpp

@@ -750,12 +750,18 @@ bool AliasAnalysis::canBuiltinDecrementRefCount(BuiltinInst *BI, SILValue Ptr) {
       continue;
 
     // A builtin can only release an object if it can escape to one of the
-    // builtin's arguments. 'EscapeAnalysis::mayReleaseContent()' expects 'Arg'
-    // to be an owned reference and disallows addresses. Conservatively handle
-    // address type arguments as and conservatively treat all other values
-    // potential owned references.
-    if (Arg->getType().isAddress() || EA->mayReleaseReferenceContent(Arg, Ptr))
-      return true;
+    // builtin's arguments.
+    if (Arg->getType().isAddress()) {
+      // Handle indirect argument as if they are a release to any references
+      // pointed to by the argument's address.
+      if (EA->mayReleaseAddressContent(Arg, Ptr))
+        return true;
+    } else {
+      // Handle direct arguments as if they are a direct release of the
+      // reference (just like a destroy_value).
+      if (EA->mayReleaseReferenceContent(Arg, Ptr))
+        return true;
+    }
   }
   return false;
 }

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -101,6 +101,54 @@ EscapeAnalysis::findRecursivePointerKind(SILType Ty,
   return NoPointer;
 }
 
+// Return the PointerKind that summarizes a class's stored properties.
+//
+// If a class only holds fields of non-pointer types, then it is guaranteed not
+// to point to any other objects.
+EscapeAnalysis::PointerKind
+EscapeAnalysis::findClassPropertiesPointerKind(SILType Ty,
+                                               const SILFunction &F) const {
+  if (Ty.isAddress())
+    return AnyPointer;
+
+  auto *classDecl = Ty.getClassOrBoundGenericClass();
+  if (!classDecl)
+    return AnyPointer;
+
+  auto &M = F.getModule();
+  auto expansion = F.getTypeExpansionContext();
+
+  // Start with the most precise pointer kind
+  PointerKind propertiesKind = NoPointer;
+  auto meetAggregateKind = [&](PointerKind otherKind) {
+    if (otherKind > propertiesKind)
+      propertiesKind = otherKind;
+  };
+  for (Type classTy = Ty.getASTType(); classTy;
+       classTy = classTy->getSuperclass()) {
+    classDecl = classTy->getClassOrBoundGenericClass();
+    assert(classDecl && "superclass must be a class");
+
+    // Return AnyPointer unless we have guaranteed visibility into all class and
+    // superclass properties. Use Minimal resilience expansion because the cache
+    // is not per-function.
+    if (classDecl->isResilient())
+      return AnyPointer;
+
+    // For each field in the class, get the pointer kind for that field. For
+    // reference-type properties, this will be ReferenceOnly. For aggregates, it
+    // will be the meet over all aggregate fields.
+    SILType objTy =
+      SILType::getPrimitiveObjectType(classTy->getCanonicalType());
+    for (VarDecl *property : classDecl->getStoredProperties()) {
+      SILType fieldTy =
+          objTy.getFieldType(property, M, expansion).getObjectType();
+      meetAggregateKind(findCachedPointerKind(fieldTy, F));
+    }
+  }
+  return propertiesKind;
+}
+
 // Returns the kind of pointer that \p Ty recursively contains.
 EscapeAnalysis::PointerKind
 EscapeAnalysis::findCachedPointerKind(SILType Ty, const SILFunction &F) const {
@@ -113,6 +161,19 @@ EscapeAnalysis::findCachedPointerKind(SILType Ty, const SILFunction &F) const {
   return pointerKind;
 }
 
+EscapeAnalysis::PointerKind
+EscapeAnalysis::findCachedClassPropertiesKind(SILType Ty,
+                                              const SILFunction &F) const {
+  auto iter = classPropertiesKindCache.find(Ty);
+  if (iter != classPropertiesKindCache.end())
+    return iter->second;
+
+  PointerKind pointerKind = findClassPropertiesPointerKind(Ty, F);
+  const_cast<EscapeAnalysis *>(this)
+    ->classPropertiesKindCache[Ty] = pointerKind;
+  return pointerKind;
+}
+
 // If EscapeAnalysis should consider the given value to be a derived address or
 // pointer based on one of its address or pointer operands, then return that
 // operand value. Otherwise, return an invalid value.
@@ -2124,11 +2185,18 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
       if (!fieldNode) {
         // In the unexpected case that the object has no field content, create
         // escaping unknown content.
+        //
+        // TODO: Why does this need to be "escaping"? The fields can't escape
+        // during deinitialization.
         ConGraph->getOrCreateUnknownContent(objNode)->markEscaping();
         return;
       }
       if (!deinitIsKnownToNotCapture(OpV)) {
-        ConGraph->getOrCreateUnknownContent(fieldNode)->markEscaping();
+        // Find out if the object's fields may have any references or pointers.
+        PointerKind propertiesKind =
+            findCachedClassPropertiesKind(OpV->getType(), *OpV->getFunction());
+        if (propertiesKind != EscapeAnalysis::NoPointer)
+          ConGraph->getOrCreateUnknownContent(fieldNode)->markEscaping();
         return;
       }
       // This deinit is known to not directly capture it's own field content;

test/SILOptimizer/arcsequenceopts.sil

@@ -630,9 +630,18 @@ bb0(%0 : $Cls):
 // 'Container' object, so the child will be destroyed with the parent.
 // The original SIL already has an over-release of the parent.
 //
+// TODO: This optimization is currently defeated because
+// - release_container marks anything '%2 = alloc_ref $Container' points to as escaping
+// - in this function, '%2 = alloc_ref $Container' points to '%1 = alloc_ref $Cls'
+// - therefore, the 'apply' may potentially release '%1 = alloc_ref $Cls'
+//
+// To hoist this 'release %1 : $Cls', AliasAnalysis alsos need to prove that
+// it isn't being hoisted above the last use. This information is not
+// properly communicated between AliasAnalysis and EscapeAnalysis.
+//
 // CHECK-LABEL: sil @remove_as_local_object_indirectly_escapes_to_callee
-// CHECK-NOT: retain_value
-// CHECK-NOT: release_value
+// CHECK: retain_value
+// CHECK: release_value
 sil @remove_as_local_object_indirectly_escapes_to_callee : $@convention(thin) (Cls) -> () {
 bb0(%0 : $Cls):
   %1 = alloc_ref $Cls

test/SILOptimizer/escape_analysis.sil

@@ -8,7 +8,7 @@ import Builtin
 import Swift
 import SwiftShims
 
-protocol ClassP : class {
+protocol ClassP : AnyObject {
   func foo()
 }
 
@@ -1774,7 +1774,7 @@ class IntWrapper {
 
 // CHECK-LABEL: CG of testInteriorCycle
 // CHECK-NEXT:   Arg [ref] %0 Esc: A, Succ: (%2)
-// CHECK-NEXT:   Con [int] %2 Esc: G, Succ: (%2)
+// CHECK-NEXT:   Con [int] %2 Esc: A, Succ: (%2)
 // CHECK-NEXT:   Val %6 Esc: , Succ: %0, %2
 // CHECK-NEXT:   Ret return Esc: , Succ: %6
 // CHECK-LABEL: End

test/SILOptimizer/late_release_hoisting.sil

@@ -17,6 +17,14 @@ class HasInt64 {
   var i : Int64
 }
 
+class HasInt64Sub : HasObj {
+  var i : Int64
+}
+
+class HasHasInt64 : HasObj {
+  var hi : HasInt64
+}
+
 class HasHasObj {
   var ho : HasObj
 }
@@ -26,10 +34,10 @@ class HasHasObj {
 //
 // CHECK-LABEL: sil @testLocalNotReachesEscaped : $@convention(thin) (Int64, @owned HasObj) -> AnyObject {
 // CHECK: bb0(%0 : $Int64, %1 : $HasObj):
-// CHECK:   [[LO:%.*]] = alloc_ref $HasInt
+// CHECK:   [[LO:%.*]] = alloc_ref $HasInt64
 // CHECK:   [[IADR:%.*]] = ref_element_addr [[LO]] : $HasInt64, #HasInt64.i
 // CHECK:   store %0 to [[IADR]] : $*Int64
-// CHECK:   strong_release [[LO]] : $HasInt
+// CHECK:   strong_release [[LO]] : $HasInt64
 // CHECK:   [[OADR:%.*]] = ref_element_addr %1 : $HasObj, #HasObj.o
 // CHECK:   [[O:%.*]] = load [[OADR]] : $*AnyObject
 // CHECK:   return [[O]] : $AnyObject
@@ -39,13 +47,39 @@ bb0(%0 : $Int64, %1 : $HasObj):
   %lo = alloc_ref $HasInt64
   %iadr = ref_element_addr %lo : $HasInt64, #HasInt64.i
   store %0 to %iadr : $*Int64
-
   %oadr = ref_element_addr %1 : $HasObj, #HasObj.o
   %o = load %oadr : $*AnyObject
   strong_release %lo : $HasInt64
   return %o : $AnyObject
 }
 
+// The release of %lo cannot be hoisted over the load because it the
+// release of %lo causes all objects pointed to by its fields to
+// escape. Since it has at least one reference-type field, it may
+// point to escaping memory.
+//
+// CHECK-LABEL: sil @testLocalMayReachEscaped : $@convention(thin) (Int64, @owned HasObj) -> AnyObject {
+// CHECK: bb0(%0 : $Int64, %1 : $HasObj):
+// CHECK:   [[LO:%.*]] = alloc_ref $HasInt64Sub
+// CHECK:   [[IADR:%.*]] = ref_element_addr [[LO]] : $HasInt64Sub, #HasInt64Sub.i
+// CHECK:   store %0 to [[IADR]] : $*Int64
+// CHECK:   [[OADR:%.*]] = ref_element_addr %1 : $HasObj, #HasObj.o
+// CHECK:   [[O:%.*]] = load [[OADR]] : $*AnyObject
+// CHECK:   strong_release [[LO]] : $HasInt64Sub
+// CHECK:   return [[O]] : $AnyObject
+// CHECK-LABEL: } // end sil function 'testLocalMayReachEscaped'
+sil @testLocalMayReachEscaped : $@convention(thin) (Int64, @owned HasObj) -> AnyObject {
+bb0(%0 : $Int64, %1 : $HasObj):
+  %lo = alloc_ref $HasInt64Sub
+  %iadr = ref_element_addr %lo : $HasInt64Sub, #HasInt64Sub.i
+  store %0 to %iadr : $*Int64
+
+  %oadr = ref_element_addr %1 : $HasObj, #HasObj.o
+  %o = load %oadr : $*AnyObject
+  strong_release %lo : $HasInt64Sub
+  return %o : $AnyObject
+}
+
 // The release of %lo cannot be hoisted over the load.
 //
 // CHECK-LABEL: sil @testLocalReachesEscaped : $@convention(thin) (@owned AnyObject) -> AnyObject {
@@ -70,10 +104,12 @@ bb0(%0 : $AnyObject):
   return %o : $AnyObject
 }
 
-// The release of %lo can be hoisted above the load from %oadr.  There
-// is a points-to relation between %lo and %oadr. However, the
-// points-to path from %lo to %oadr reaches another reference counted
-// object before reaching $oadr.
+// The release of %lo cannot be hoisted above the load from %oadr
+// because there is a points-to relation between %lo and %oadr.
+//
+// TODO: To hoist this release, AliasAnalysis also need to prove that
+// it isn't being hoisted above the last use. This information is not
+// properly communicated between AliasAnalysis and EscapeAnalysis.
 //
 // CHECK-LABEL: sil @testLocalReachesRCEscaped : $@convention(thin) (@owned HasObj) -> AnyObject {
 // CHECK: bb0(%0 : $HasObj):
@@ -82,9 +118,9 @@ bb0(%0 : $AnyObject):
 // CHECK:   strong_retain %0 : $HasObj
 // CHECK:   store %0 to [[HOADR]] : $*HasObj
 // CHECK:   [[HO:%.*]] = load [[HOADR]] : $*HasObj
-// CHECK:   strong_release [[LO]] : $HasHasObj
 // CHECK:   [[OADR:%.*]] = ref_element_addr [[HO]] : $HasObj, #HasObj.o
 // CHECK:   [[O:%.*]] = load [[OADR]] : $*AnyObject
+// CHECK:   strong_release [[LO]] : $HasHasObj
 // CHECK:   return [[O]] : $AnyObject
 // CHECK-LABEL: } // end sil function 'testLocalReachesRCEscaped'
 sil @testLocalReachesRCEscaped : $@convention(thin) (@owned HasObj) -> AnyObject {
@@ -144,9 +180,13 @@ bb0(%0 : $AnyObject):
   return %o : $AnyObject
 }
 
-// Two local references, one reachable from the other. We assume that
+// Two local references, one reachable from the other. We cannot assume that
 // the reachable one has its own reference count and hoist
-// strong_release %hho above load %oadr.
+// strong_release %hho above load %oadr without more information.
+//
+// TODO: To hoist this release, AliasAnalysis also need to prove that
+// it isn't being hoisted above the last use. This information is not
+// properly communicated between AliasAnalysis and EscapeAnalysis.
 //
 // CHECK-LABEL: sil @testLocalReachesRCLocal : $@convention(thin) (AnyObject) -> AnyObject {
 // CHECK: bb0(%0 : $AnyObject):
@@ -158,9 +198,9 @@ bb0(%0 : $AnyObject):
 // CHECK: [[HOADR:%.*]] = ref_element_addr [[HHO]] : $HasHasObj, #HasHasObj.ho
 // CHECK: store [[LO]] to [[HOADR]] : $*HasObj
 // CHECK: [[HO:%.*]] = load [[HOADR]] : $*HasObj
-// CHECK: strong_release [[HHO]] : $HasHasObj
 // CHECK: [[OADR2:%.*]] = ref_element_addr [[HO]] : $HasObj, #HasObj.o
 // CHECK: [[O:%.*]] = load [[OADR2]] : $*AnyObject
+// CHECK: strong_release [[HHO]] : $HasHasObj
 // CHECK: strong_retain [[O]] : $AnyObject
 // CHECK: return [[O]] : $AnyObject
 // CHECK-LABEL: } // end sil function 'testLocalReachesRCLocal'
@@ -232,3 +272,33 @@ bb0(%0 : $AnyObject, %1 : $HasHasObj):
   strong_release %1 : $HasHasObj
   return %o : $AnyObject
 }
+
+// 'strong_release %lo : $HasHasInt64' cannot be hoisted because it
+// contains a reference that appears to escape. With the current
+// information, we can't be sure if that escaping reference points to
+// the load of '%oadr'.
+//
+// TODO: To hoist this release, AliasAnalysis also need to prove that
+// it isn't being hoisted above the last use. This information is not
+// properly communicated between AliasAnalysis and EscapeAnalysis.
+//
+// CHECK-LABEL: sil @testLocalWithReferenceProperty : $@convention(thin) (Int64, @owned HasObj) -> AnyObject {
+// CHECK: bb0(%0 : $Int64, %1 : $HasObj):
+// CHECK:   [[O:%.*]] = load {{.*}} : $*AnyObject
+// CHECK:   strong_release %{{.*}} : $HasHasInt64
+// CHECK:   return [[O]] : $AnyObject
+// CHECK-LABEL: } // end sil function 'testLocalWithReferenceProperty'
+sil @testLocalWithReferenceProperty : $@convention(thin) (Int64, @owned HasObj) -> AnyObject {
+bb0(%0 : $Int64, %1 : $HasObj):
+  %hi = alloc_ref $HasInt64
+  %iadr = ref_element_addr %hi : $HasInt64, #HasInt64.i
+  store %0 to %iadr : $*Int64
+  %lo = alloc_ref $HasHasInt64
+  %hiadr = ref_element_addr %lo : $HasHasInt64, #HasHasInt64.hi
+  store %hi to %hiadr : $*HasInt64
+
+  %oadr = ref_element_addr %1 : $HasObj, #HasObj.o
+  %o = load %oadr : $*AnyObject
+  strong_release %lo : $HasHasInt64
+  return %o : $AnyObject
+}