ARCAnalysis: fix canNeverUseObject to correctly handle builtins.

This analysis helper was inverting the result for builtins. Builtins
such as "copyMemory" were treated as never using a value.

This manifested in a crash in TestFoundation. NSDictionary's
initializer released the incoming array before copying it. This
crashed later during dictionary destruction.

The crash was hidden by a secondary bug in mayHaveSymmetricInterference
that effectively ignored the result from canNeverUseValue.

Rename the helper to canUseObject, invert the result for builtins, and
fix mayHaveSymmetricInterference to respect the result of
canUseObject.

Note that instructions that cannot access a referenced object
obviously cannot not "interfere" with a release.

Fixing these bugs now allows ARC optimization around dealloc_stack and
other operations that don't care about the reference count.

Author: atrick

include/swift/SILOptimizer/Analysis/ARCAnalysis.h

@@ -72,12 +72,13 @@ bool mustGuaranteedUseValue(SILInstruction *User, SILValue Ptr,
 /// Returns true if \p Inst can never conservatively decrement reference counts.
 bool canNeverDecrementRefCounts(SILInstruction *Inst);
 
-/// \returns True if \p User can never use a value in a way that requires the
-/// value to be alive.
+/// Returns true if \p Inst may access any indirect object either via an address
+/// or reference.
 ///
-/// This is purposefully a negative query to contrast with canUseValue which is
-/// about a specific value while this is about general values.
-bool canNeverUseValues(SILInstruction *User);
+/// If false is returned and \p Inst has an address or reference type operand,
+/// then \p Inst only operates on the value of the address itself, not the
+/// memory. i.e. it does not dereference the address.
+bool canUseObject(SILInstruction *Inst);
 
 /// \returns true if the user \p User may use \p Ptr in a manner that requires
 /// Ptr's life to be guaranteed to exist at this point.

lib/SILOptimizer/ARC/ARCRegionState.cpp

@@ -531,7 +531,9 @@ void ARCRegionState::summarizeBlock(SILBasicBlock *BB) {
   SummarizedInterestingInsts.clear();
 
   for (auto &I : *BB)
-    if (!canNeverUseValues(&I) || I.mayReleaseOrReadRefCount() ||
+    // FIXME: mayReleaseOrReadRefCount should be a strict subset of
+    // canUseObject. If not, there is a bug in canUseObject.
+    if (canUseObject(&I) || I.mayReleaseOrReadRefCount() ||
         isStrongEntranceInstruction(I))
       SummarizedInterestingInsts.push_back(&I);
 }

lib/SILOptimizer/Analysis/ARCAnalysis.cpp

@@ -91,7 +91,7 @@ bool swift::mayDecrementRefCount(SILInstruction *User,
 //                                Use Analysis
 //===----------------------------------------------------------------------===//
 
-/// Returns true if a builtin apply cannot use reference counted values.
+/// Returns true if a builtin apply can use reference counted values.
 ///
 /// The main case that this handles here are builtins that via read none imply
 /// that they cannot read globals and at the same time do not take any
@@ -106,28 +106,33 @@ static bool canApplyOfBuiltinUseNonTrivialValues(BuiltinInst *BInst) {
     if (II.hasAttribute(llvm::Attribute::ReadNone)) {
       for (auto &Op : BInst->getAllOperands()) {
         if (!Op.get()->getType().isTrivial(*F)) {
-          return false;
+          return true;
         }
       }
     }
 
-    return true;
+    return false;
   }
 
   auto &BI = BInst->getBuiltinInfo();
-  if (BI.isReadNone()) {
-    for (auto &Op : BInst->getAllOperands()) {
-      if (!Op.get()->getType().isTrivial(*F)) {
-        return false;
-      }
+  if (!BI.isReadNone())
+    return true;
+
+  for (auto &Op : BInst->getAllOperands()) {
+    if (!Op.get()->getType().isTrivial(*F)) {
+      return true;
     }
   }
-
-  return true;
+  return false;
 }
 
-/// Returns true if Inst is a function that we know never uses ref count values.
-bool swift::canNeverUseValues(SILInstruction *Inst) {
+/// Returns true if \p Inst may access any indirect object either via an address
+/// or reference.
+///
+/// If these instructions do have an address or reference type operand, then
+/// they only operate on the value of the address itself, not the
+/// memory. i.e. they don't dereference the address.
+bool swift::canUseObject(SILInstruction *Inst) {
   switch (Inst->getKind()) {
   // These instructions do not use other values.
   case SILInstructionKind::FunctionRefInst:
@@ -142,33 +147,36 @@ bool swift::canNeverUseValues(SILInstruction *Inst) {
   case SILInstructionKind::AllocBoxInst:
   case SILInstructionKind::MetatypeInst:
   case SILInstructionKind::WitnessMethodInst:
-    return true;
+    return false;
 
   // DeallocStackInst do not use reference counted values.
   case SILInstructionKind::DeallocStackInst:
-    return true;
+    return false;
 
   // Debug values do not use referenced counted values in a manner we care
   // about.
   case SILInstructionKind::DebugValueInst:
   case SILInstructionKind::DebugValueAddrInst:
-    return true;
+    return false;
 
   // Casts do not use pointers in a manner that we care about since we strip
   // them during our analysis. The reason for this is if the cast is not dead
   // then there must be some other use after the cast that we will protect if a
   // release is not in between the cast and the use.
+  //
+  // Note: UncheckedRefCastAddrInst moves a reference into a new object. While
+  // the net reference count should be zero, there's no guarantee it won't
+  // access the object.
   case SILInstructionKind::UpcastInst:
   case SILInstructionKind::AddressToPointerInst:
   case SILInstructionKind::PointerToAddressInst:
   case SILInstructionKind::UncheckedRefCastInst:
-  case SILInstructionKind::UncheckedRefCastAddrInst:
   case SILInstructionKind::UncheckedAddrCastInst:
   case SILInstructionKind::RefToRawPointerInst:
   case SILInstructionKind::RawPointerToRefInst:
   case SILInstructionKind::UnconditionalCheckedCastInst:
   case SILInstructionKind::UncheckedBitwiseCastInst:
-    return true;
+    return false;
 
   // If we have a trivial bit cast between trivial types, it is not something
   // that can use ref count ops in a way we care about. We do need to be careful
@@ -183,7 +191,7 @@ bool swift::canNeverUseValues(SILInstruction *Inst) {
   // safe.
   case SILInstructionKind::UncheckedTrivialBitCastInst: {
     SILValue Op = cast<UncheckedTrivialBitCastInst>(Inst)->getOperand();
-    return Op->getType().isTrivial(*Inst->getFunction());
+    return !Op->getType().isTrivial(*Inst->getFunction());
   }
 
   // Typed GEPs do not use pointers. The user of the typed GEP may but we will
@@ -198,18 +206,18 @@ bool swift::canNeverUseValues(SILInstruction *Inst) {
   case SILInstructionKind::UncheckedEnumDataInst:
   case SILInstructionKind::IndexAddrInst:
   case SILInstructionKind::IndexRawPointerInst:
-      return true;
+    return false;
 
   // Aggregate formation by themselves do not create new uses since it is their
   // users that would create the appropriate uses.
   case SILInstructionKind::EnumInst:
   case SILInstructionKind::StructInst:
   case SILInstructionKind::TupleInst:
-    return true;
+    return false;
 
   // Only uses non reference counted values.
   case SILInstructionKind::CondFailInst:
-    return true;
+    return false;
 
   case SILInstructionKind::BuiltinInst: {
     auto *BI = cast<BuiltinInst>(Inst);
@@ -221,9 +229,9 @@ bool swift::canNeverUseValues(SILInstruction *Inst) {
   // dead, LLVM will clean it up.
   case SILInstructionKind::BranchInst:
   case SILInstructionKind::CondBranchInst:
-    return true;
-  default:
     return false;
+  default:
+    return true;
   }
 }
 
@@ -268,14 +276,15 @@ static bool canTerminatorUseValue(TermInst *TI, SILValue Ptr,
 
 
 bool swift::mayHaveSymmetricInterference(SILInstruction *User, SILValue Ptr, AliasAnalysis *AA) {
+  // If Inst is an instruction that we know can never use values with reference
+  // semantics, return true. Check this before AliasAnalysis because some memory
+  // operations, like dealloc_stack, don't use ref counted values.
+  if (!canUseObject(User))
+    return false;
+
   // Check whether releasing this value can call deinit and interfere with User.
   if (AA->mayValueReleaseInterfereWithInstruction(User, Ptr))
     return true;
-  
-  // If Inst is an instruction that we know can never use values with reference
-  // semantics, return true.
-  if (canNeverUseValues(User))
-    return false;
 
   // If the user is a load or a store and we can prove that it does not access
   // the object then return true.

test/SILOptimizer/existential_transform.swift

@@ -301,8 +301,8 @@ func wrap_gcp<T:GP>(_ a:T,_ b:GP) -> Int {
 // CHECK: strong_retain 
 // CHECK: apply
 // CHECK: destroy_addr 
-// CHECK: dealloc_stack
 // CHECK: strong_release 
+// CHECK: dealloc_stack
 // CHECK: return 
 // CHECK: } // end sil function '$s21existential_transform3gcpySixAA2GPRzlF'
 @inline(never) func gcp<T:GP>(_ a:T) -> Int {
@@ -328,8 +328,8 @@ func wrap_gcp_arch<T:GP>(_ a:T,_ b:GP, _ c:inout Array<T>) -> Int {
 // CHECK: strong_retain
 // CHECK: apply
 // CHECK: destroy_addr
-// CHECK: dealloc_stack
 // CHECK: strong_release
+// CHECK: dealloc_stack
 // CHECK: return
 // CHECK-LABEL: } // end sil function '$s21existential_transform8gcp_archySix_SayxGztAA2GPRzlF'
 @inline(never) func gcp_arch<T:GP>(_ a:T, _ b:inout Array<T>) -> Int {

test/SILOptimizer/retain_release_code_motion.sil

@@ -718,3 +718,43 @@ bb2:
   return %9999 : $()
 }
 
+// Hoist releases above dealloc_stack
+// CHECK-LABEL: sil @testReleaseHoistDeallocStack : $@convention(thin) (AnyObject) -> () {
+// CHECK: bb0(%0 : $AnyObject):
+// CHECK-NOT:  retain
+// CHECK:      [[A:%.*]] = alloc_stack $Int64
+// CHECK-NEXT  dealloc_stack [[A]] : $*Int64
+// CHECK-NOT:  release
+// CHECK-LABEL: } // end sil function 'testReleaseHoistDeallocStack'
+sil @testReleaseHoistDeallocStack : $@convention(thin) (AnyObject)->() {
+bb0(%0 : $AnyObject):
+  strong_retain %0 : $AnyObject
+  %alloc = alloc_stack $Int64
+  dealloc_stack %alloc : $*Int64
+  strong_release %0 : $AnyObject
+  %34 = tuple ()
+  return %34 : $()
+}
+
+// Do not hoist releases above builtins that operate on object references.
+//
+// CHECK-RELEASE-HOISTING-LABEL: sil @testCopyArray : $@convention(thin) (_ContiguousArrayBuffer<AnyObject>, Builtin.Word, Builtin.Word) -> Builtin.RawPointer {
+// CHECK-RELEASE-HOISTING: bb0(%0 : $_ContiguousArrayBuffer<AnyObject>, %1 : $Builtin.Word, %2 : $Builtin.Word):
+// CHECK-RELEASE-HOISTING:   builtin "copyArray"<AnyObject>
+// CHECK-RELEASE-HOISTING:   release_value %0 : $_ContiguousArrayBuffer<AnyObject>
+// CHECK-RELEASE-HOISTING-LABEL: } // end sil function 'testCopyArray'
+sil @testCopyArray : $@convention(thin) (_ContiguousArrayBuffer<AnyObject>, Builtin.Word, Builtin.Word) -> Builtin.RawPointer {
+bb0(%0 : $_ContiguousArrayBuffer<AnyObject>, %1 : $Builtin.Word, %2 : $Builtin.Word):
+  %eltty = metatype $@thick AnyObject.Protocol
+  %newptr = builtin "allocRaw"(%1 : $Builtin.Word, %1 : $Builtin.Word) : $Builtin.RawPointer
+  bind_memory %newptr : $Builtin.RawPointer, %1 : $Builtin.Word to $*AnyObject
+  %storage = struct_extract %0 : $_ContiguousArrayBuffer<AnyObject>, #_ContiguousArrayBuffer._storage
+  %elements = ref_tail_addr %storage : $__ContiguousArrayStorageBase, $AnyObject
+  %eltptr = address_to_pointer %elements : $*AnyObject to $Builtin.RawPointer
+  %objptr = struct $UnsafePointer<AnyObject> (%eltptr : $Builtin.RawPointer)
+  %ptrdep = mark_dependence %objptr : $UnsafePointer<AnyObject> on %storage : $__ContiguousArrayStorageBase
+  %rawptr = struct_extract %ptrdep : $UnsafePointer<AnyObject>, #UnsafePointer._rawValue
+  %copy = builtin "copyArray"<AnyObject>(%eltty : $@thick AnyObject.Protocol, %newptr : $Builtin.RawPointer, %rawptr : $Builtin.RawPointer, %1 : $Builtin.Word) : $()
+  release_value %0 : $_ContiguousArrayBuffer<AnyObject>
+  return %newptr : $Builtin.RawPointer
+}