Merge pull request swiftlang#35737 from eeckstein/doe-destroy-array

Author: swift-ci

include/swift/SIL/MemAccessUtils.h

@@ -821,6 +821,10 @@ class AccessPath {
   /// access, thus not within an access scope.
   static AccessPath computeInScope(SILValue address);
 
+  /// Creates an AccessPass, which identifies the first tail-element of the
+  /// object \p rootReference.
+  static AccessPath forTailStorage(SILValue rootReference);
+
   // Encode a dynamic index_addr as an UnknownOffset.
   static constexpr int UnknownOffset = std::numeric_limits<int>::min() >> 1;
 
@@ -939,7 +943,8 @@ class AccessPath {
 
   bool operator==(AccessPath other) const {
     return
-      storage.hasIdenticalBase(other.storage) && pathNode == other.pathNode;
+      storage.hasIdenticalBase(other.storage) && pathNode == other.pathNode &&
+      offset == other.offset;
   }
   bool operator!=(AccessPath other) const { return !(*this == other); }
 
@@ -984,6 +989,12 @@ class AccessPath {
               SILFunction *function,
               unsigned useLimit = std::numeric_limits<unsigned>::max()) const;
 
+  /// Returns a new AccessPass, identical to this AccessPath, except that the
+  /// offset is replaced with \p newOffset.
+  AccessPath withOffset(int newOffset) const {
+    return AccessPath(storage, pathNode, newOffset);
+  }
+
   void printPath(raw_ostream &os) const;
   void print(raw_ostream &os) const;
   void dump() const;

lib/SIL/Utils/MemAccessUtils.cpp

@@ -763,6 +763,13 @@ AccessedStorage AccessedStorage::computeInScope(SILValue sourceAddress) {
 //                              MARK: AccessPath
 //===----------------------------------------------------------------------===//
 
+AccessPath AccessPath::forTailStorage(SILValue rootReference) {
+  return AccessPath(
+    AccessedStorage::forClass(rootReference, AccessedStorage::TailIndex),
+    PathNode(rootReference->getModule()->getIndexTrieRoot()),
+    /*offset*/ 0);
+}
+
 bool AccessPath::contains(AccessPath subPath) const {
   if (!isValid() || !subPath.isValid()) {
     return false;

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -997,6 +997,31 @@ SILInstruction *SILCombiner::visitIndexAddrInst(IndexAddrInst *IA) {
   return Builder.createIndexAddr(IA->getLoc(), base2, newIndex);
 }
 
+/// Walks over all fields of an aggregate and checks if a reference count
+/// operation for \p value is required. This differs from a simple `isTrivial`
+/// check, because it treats a value_to_bridge_object instruction as "trivial".
+static bool isTrivial(SILValue value, SILFunction *function) {
+  SmallVector<ValueBase *, 32> workList;
+  SmallPtrSet<ValueBase *, 16> visited;
+  workList.push_back(value);
+  while (!workList.empty()) {
+    SILValue v = workList.pop_back_val();
+    if (v->getType().isTrivial(*function))
+      continue;
+    if (isa<ValueToBridgeObjectInst>(v))
+      continue;
+    if (isa<StructInst>(v) || isa<TupleInst>(v)) {
+      for (SILValue op : cast<SingleValueInstruction>(v)->getOperandValues()) {
+        if (visited.insert(op).second)
+          workList.push_back(op);
+      }
+      continue;
+    }
+    return false;
+  }
+  return true;
+}
+
 SILInstruction *SILCombiner::visitReleaseValueInst(ReleaseValueInst *RVI) {
   assert(!RVI->getFunction()->hasOwnership());
 
@@ -1031,7 +1056,7 @@ SILInstruction *SILCombiner::visitReleaseValueInst(ReleaseValueInst *RVI) {
                                        RVI->getAtomicity());
 
   // ReleaseValueInst of a trivial type is a no-op.
-  if (OperandTy.isTrivial(*RVI->getFunction()))
+  if (isTrivial(Operand, RVI->getFunction()))
     return eraseInstFromFunction(*RVI);
 
   // Do nothing for non-trivial non-reference types.

lib/SILOptimizer/Transforms/DeadObjectElimination.cpp

@@ -36,6 +36,7 @@
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/SILModule.h"
 #include "swift/SIL/SILUndef.h"
+#include "swift/SIL/MemAccessUtils.h"
 #include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
@@ -248,18 +249,112 @@ static bool canZapInstruction(SILInstruction *Inst, bool acceptRefCountInsts,
   return false;
 }
 
+/// Returns true if all stores in \p users store to the tail elements of
+/// \p allocRef, which are destroyed by the \p destroyArray builtin.
+static bool onlyStoresToTailObjects(BuiltinInst *destroyArray,
+                                    const UserList &users,
+                                    AllocRefInst *allocRef) {
+  // Get the number of destroyed elements.
+  auto *literal = dyn_cast<IntegerLiteralInst>(destroyArray->getArguments()[2]);
+  if (!literal || literal->getValue().getMinSignedBits() > 32)
+    return false;
+  int numDestroyed = literal->getValue().getSExtValue();
+  
+  SILFunction *func = destroyArray->getFunction();
+  SILBasicBlock *storesBlock = nullptr;
+
+  // Check if the destroyArray destroys the tail elements of allocRef.
+  auto destroyPath = AccessPath::compute(destroyArray->getArguments()[1]);
+  if (destroyPath != AccessPath::forTailStorage(allocRef))
+    return false;
+
+  SmallVector<AccessPath, 32> pathsToCheck;
+
+  // Check all stores to the tail elements.
+  for (SILInstruction *user : users) {
+    auto *store = dyn_cast<StoreInst>(user);
+    if (!store)
+      continue;
+
+    assert(users.count(store->getSrc()->getDefiningInstruction()) == 0 &&
+           "Storing a use of an array (that would mean the array escapes)?");
+
+    // All stores must be in the same block. This ensure that the stores
+    // dominate the destroyArray (which may be in a different block).
+    if (storesBlock && store->getParent() != storesBlock)
+      return false;
+    storesBlock = store->getParent();
+
+    AccessPath storePath = AccessPath::compute(store->getDest());
+    if (!storePath.isValid())
+      return false;
+
+    // We don't care about trivial stores.
+    if (store->getSrc()->getType().isTrivial(*func))
+      continue;
+    
+    // Check if it's a store to the tail elements.
+    if (!destroyPath.contains(storePath.withOffset(0)))
+      return false;
+
+    // Check if the store is within the range of the destroyed array. In OSSA
+    // we would not need this check. Otherwise it would be a memory lifetime
+    // failure.
+    if (storePath.getOffset() < 0 || storePath.getOffset() >= numDestroyed)
+      return false;
+
+    pathsToCheck.push_back(storePath);
+  }
+
+  // In non-OSSA we have to check if two paths overlap, because we could end up
+  // over-releasing the stored objects.
+  // Group the paths by tail-element index, so that we only have to check within
+  // a tail-element group.
+  std::sort(pathsToCheck.begin(), pathsToCheck.end(), [](AccessPath p1, AccessPath p2) {
+    return p1.getOffset() < p2.getOffset();
+  });
+  for (unsigned i = 0, n = pathsToCheck.size(); i < n; ++i) {
+    for (unsigned j = i + 1;
+      j < n && pathsToCheck[i].getOffset() == pathsToCheck[j].getOffset(); ++j) {
+      if (pathsToCheck[i].mayOverlap(pathsToCheck[j]))
+        return false;
+      // Limit the number of checks to avoid quadratic complexity.
+      if (j > i + 8)
+        return false;
+    }
+  }
+  return true;
+}
+
+static bool isDestroyArray(SILInstruction *inst) {
+  BuiltinInst *bi = dyn_cast<BuiltinInst>(inst);
+  return bi && bi->getBuiltinInfo().ID == BuiltinValueKind::DestroyArray;
+}
+
+/// Inserts releases of all stores in \p users.
+static void insertCompensatingReleases(SILInstruction *before,
+                                       const UserList &users) {
+  for (SILInstruction *user : users) {
+    if (auto *store = dyn_cast<StoreInst>(user)) {
+      createDecrementBefore(store->getSrc(), before);
+    }
+  }
+}
+
 /// Analyze the use graph of AllocRef for any uses that would prevent us from
 /// zapping it completely.
 static bool
-hasUnremovableUsers(SILInstruction *AllocRef, UserList *Users,
+hasUnremovableUsers(SILInstruction *allocation, UserList *Users,
                     bool acceptRefCountInsts, bool onlyAcceptTrivialStores) {
   SmallVector<SILInstruction *, 16> Worklist;
-  Worklist.push_back(AllocRef);
+  Worklist.push_back(allocation);
 
   LLVM_DEBUG(llvm::dbgs() << "    Analyzing Use Graph.");
 
   SmallVector<RefElementAddrInst *, 8> refElementAddrs;
   bool deallocationMaybeInlined = false;
+  BuiltinInst *destroyArray = nullptr;
+  AllocRefInst *allocRef = dyn_cast<AllocRefInst>(allocation);
 
   while (!Worklist.empty()) {
     SILInstruction *I = Worklist.pop_back_val();
@@ -273,17 +368,19 @@ hasUnremovableUsers(SILInstruction *AllocRef, UserList *Users,
       continue;
     }
 
-    // If we can't zap this instruction... bail...
-    if (!canZapInstruction(I, acceptRefCountInsts, onlyAcceptTrivialStores)) {
-      LLVM_DEBUG(llvm::dbgs() << "        Found instruction we can't zap...\n");
-      return true;
-    }
-
     if (auto *rea = dyn_cast<RefElementAddrInst>(I)) {
       if (!rea->getType().isTrivial(*rea->getFunction()))
         refElementAddrs.push_back(rea);
     } else if (isa<SetDeallocatingInst>(I)) {
       deallocationMaybeInlined = true;
+    } else if (allocRef && Users && isDestroyArray(I)) {
+      if (destroyArray)
+        return true;
+      destroyArray = cast<BuiltinInst>(I);
+    } else if (!canZapInstruction(I, acceptRefCountInsts,
+                                  onlyAcceptTrivialStores)) {
+      LLVM_DEBUG(llvm::dbgs() << "        Found instruction we can't zap...\n");
+      return true;
     }
 
     // At this point, we can remove the instruction as long as all of its users
@@ -309,6 +406,12 @@ hasUnremovableUsers(SILInstruction *AllocRef, UserList *Users,
     }
   }
 
+  // In OSSA, we don't have to do this check. We can always accept a
+  // destroyArray and insert the compensating destroys right at the store
+  // instructions.
+  if (destroyArray)
+    return !onlyStoresToTailObjects(destroyArray, *Users, allocRef);
+
   if (deallocationMaybeInlined) {
     // The alloc_ref is not destructed by a strong_release which is calling the
     // deallocator (destroying all stored properties).
@@ -767,6 +870,11 @@ bool DeadObjectElimination::processAllocRef(AllocRefInst *ARI) {
     return false;
   }
 
+  auto iter = std::find_if(UsersToRemove.begin(), UsersToRemove.end(),
+                           isDestroyArray);
+  if (iter != UsersToRemove.end())
+    insertCompensatingReleases(*iter, UsersToRemove);
+
   // Remove the AllocRef and all of its users.
   removeInstructions(
     ArrayRef<SILInstruction*>(UsersToRemove.begin(), UsersToRemove.end()));