Merge pull request #32130 from gottesmm/release/5.3-rdar63188362

[5.3] [semantic-arc-opts] Use all consuming uses instead of just destroying uses when validating if a LiveRange is alive in a scope.

Author: tkremenek

lib/SILOptimizer/Transforms/SemanticARCOpts.cpp

@@ -52,21 +52,36 @@ class LiveRange {
   /// introducer and not to be forwarding.
   OwnedValueIntroducer introducer;
 
+  /// A vector that we store all of our uses into.
+  ///
+  /// Some properties of this array are:
+  ///
+  /// 1. It is only mutated in the constructor of LiveRange.
+  ///
+  /// 2. destroyingUses, ownershipForwardingUses, and unknownConsumingUses are
+  /// views into this array. We store the respective uses in the aforementioned
+  /// order. This is why it is important not to mutate consumingUses after we
+  /// construct the LiveRange since going from small -> large could invalidate
+  /// the uses.
+  SmallVector<Operand *, 6> consumingUses;
+
   /// A list of destroy_values of the live range.
-  SmallVector<Operand *, 2> destroyingUses;
+  ///
+  /// This is just a view into consuming uses.
+  ArrayRef<Operand *> destroyingUses;
 
   /// A list of forwarding instructions that forward owned ownership, but that
   /// are also able to be converted to guaranteed ownership. If we are able to
   /// eliminate this LiveRange due to it being from a guaranteed value, we must
   /// flip the ownership of all of these instructions to guaranteed from owned.
   ///
   /// Corresponds to isOwnershipForwardingInst(...).
-  SmallVector<Operand *, 2> ownershipForwardingUses;
+  ArrayRef<Operand *> ownershipForwardingUses;
 
   /// Consuming uses that we were not able to understand as a forwarding
   /// instruction or a destroy_value. These must be passed a strongly control
   /// equivalent +1 value.
-  SmallVector<Operand *, 2> unknownConsumingUses;
+  ArrayRef<Operand *> unknownConsumingUses;
 
 public:
   LiveRange(SILValue value);
@@ -87,6 +102,11 @@ class LiveRange {
   HasConsumingUse_t
   hasUnknownConsumingUse(bool assumingFixedPoint = false) const;
 
+  /// Return an array ref to /all/ consuming uses. Will include all 3 sorts of
+  /// consuming uses: destroying uses, forwarding consuming uses, and unknown
+  /// forwarding instruction.
+  ArrayRef<Operand *> getAllConsumingUses() const { return consumingUses; }
+
   ArrayRef<Operand *> getDestroyingUses() const { return destroyingUses; }
 
 private:
@@ -121,7 +141,7 @@ class LiveRange {
     return ownershipForwardingUses;
   }
 
-  void convertOwnedGeneralForwardingUsesToGuaranteed();
+  void convertOwnedGeneralForwardingUsesToGuaranteed() &&;
 
   /// A consuming operation that:
   ///
@@ -192,6 +212,10 @@ LiveRange::LiveRange(SILValue value)
       ownershipForwardingUses(), unknownConsumingUses() {
   assert(introducer.value.getOwnershipKind() == ValueOwnershipKind::Owned);
 
+  SmallVector<Operand *, 32> tmpDestroyingUses;
+  SmallVector<Operand *, 32> tmpForwardingConsumingUses;
+  SmallVector<Operand *, 32> tmpUnknownConsumingUses;
+
   // We know that our silvalue produces an @owned value. Look through all of our
   // uses and classify them as either consuming or not.
   SmallVector<Operand *, 32> worklist(introducer.value->getUses());
@@ -220,7 +244,7 @@ LiveRange::LiveRange(SILValue value)
     // Ok, we know now that we have a consuming use. See if we have a destroy
     // value, quickly up front. If we do have one, stash it and continue.
     if (isa<DestroyValueInst>(user)) {
-      destroyingUses.push_back(op);
+      tmpDestroyingUses.push_back(op);
       continue;
     }
 
@@ -244,13 +268,13 @@ LiveRange::LiveRange(SILValue value)
                         return v.getOwnershipKind() ==
                                ValueOwnershipKind::Owned;
                       })) {
-      unknownConsumingUses.push_back(op);
+      tmpUnknownConsumingUses.push_back(op);
       continue;
     }
 
     // Ok, this is a forwarding instruction whose ownership we can flip from
     // owned -> guaranteed.
-    ownershipForwardingUses.push_back(op);
+    tmpForwardingConsumingUses.push_back(op);
 
     // If we have a non-terminator, just visit its users recursively to see if
     // the the users force the live range to be alive.
@@ -284,6 +308,20 @@ LiveRange::LiveRange(SILValue value)
       }
     }
   }
+
+  // The order in which we append these to consumingUses matters since we assume
+  // their order as an invariant. This is done to ensure that we can pass off
+  // all of our uses or individual sub-arrays of our users without needing to
+  // move around memory.
+  llvm::copy(tmpDestroyingUses, std::back_inserter(consumingUses));
+  llvm::copy(tmpForwardingConsumingUses, std::back_inserter(consumingUses));
+  llvm::copy(tmpUnknownConsumingUses, std::back_inserter(consumingUses));
+
+  auto cUseArrayRef = llvm::makeArrayRef(consumingUses);
+  destroyingUses = cUseArrayRef.take_front(tmpDestroyingUses.size());
+  ownershipForwardingUses = cUseArrayRef.slice(
+      tmpDestroyingUses.size(), tmpForwardingConsumingUses.size());
+  unknownConsumingUses = cUseArrayRef.take_back(tmpUnknownConsumingUses.size());
 }
 
 void LiveRange::insertEndBorrowsAtDestroys(
@@ -339,9 +377,10 @@ void LiveRange::insertEndBorrowsAtDestroys(
   }
 }
 
-void LiveRange::convertOwnedGeneralForwardingUsesToGuaranteed() {
+void LiveRange::convertOwnedGeneralForwardingUsesToGuaranteed() && {
   while (!ownershipForwardingUses.empty()) {
-    auto *i = ownershipForwardingUses.pop_back_val()->getUser();
+    auto *i = ownershipForwardingUses.back()->getUser();
+    ownershipForwardingUses = ownershipForwardingUses.drop_back();
 
     // If this is a term inst, just convert all of its incoming values that are
     // owned to be guaranteed.
@@ -402,7 +441,8 @@ void LiveRange::convertToGuaranteedAndRAUW(SILValue newGuaranteedValue,
                                            InstModCallbacks callbacks) && {
   auto *value = cast<SingleValueInstruction>(introducer.value);
   while (!destroyingUses.empty()) {
-    auto *d = destroyingUses.pop_back_val();
+    auto *d = destroyingUses.back();
+    destroyingUses = destroyingUses.drop_back();
     callbacks.deleteInst(d->getUser());
     ++NumEliminatedInsts;
   }
@@ -412,7 +452,7 @@ void LiveRange::convertToGuaranteedAndRAUW(SILValue newGuaranteedValue,
   // Then change all of our guaranteed forwarding insts to have guaranteed
   // ownership kind instead of what ever they previously had (ignoring trivial
   // results);
-  convertOwnedGeneralForwardingUsesToGuaranteed();
+  std::move(*this).convertOwnedGeneralForwardingUsesToGuaranteed();
 }
 
 void LiveRange::convertArgToGuaranteed(DeadEndBlocks &deadEndBlocks,
@@ -429,15 +469,16 @@ void LiveRange::convertArgToGuaranteed(DeadEndBlocks &deadEndBlocks,
 
   // Then eliminate all of the destroys...
   while (!destroyingUses.empty()) {
-    auto *d = destroyingUses.pop_back_val();
+    auto *d = destroyingUses.back();
+    destroyingUses = destroyingUses.drop_back();
     callbacks.deleteInst(d->getUser());
     ++NumEliminatedInsts;
   }
 
   // and change all of our guaranteed forwarding insts to have guaranteed
   // ownership kind instead of what ever they previously had (ignoring trivial
   // results);
-  convertOwnedGeneralForwardingUsesToGuaranteed();
+  std::move(*this).convertOwnedGeneralForwardingUsesToGuaranteed();
 }
 
 LiveRange::HasConsumingUse_t
@@ -1297,8 +1338,9 @@ bool SemanticARCOptVisitor::performGuaranteedCopyValueOptimization(CopyValueInst
     SmallVector<Operand *, 8> scratchSpace;
     SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
     if (llvm::any_of(borrowScopeIntroducers, [&](BorrowedValue borrowScope) {
-          return !borrowScope.areUsesWithinScope(
-              destroys, scratchSpace, visitedBlocks, getDeadEndBlocks());
+          return !borrowScope.areUsesWithinScope(lr.getAllConsumingUses(),
+                                                 scratchSpace, visitedBlocks,
+                                                 getDeadEndBlocks());
         })) {
       return false;
     }
@@ -1330,12 +1372,11 @@ bool SemanticARCOptVisitor::performGuaranteedCopyValueOptimization(CopyValueInst
         return false;
       }
 
-      if (llvm::any_of(borrowScopeIntroducers,
-                       [&](BorrowedValue borrowScope) {
-                         return !borrowScope.areUsesWithinScope(
-                             phiArgLR.getDestroyingUses(), scratchSpace,
-                             visitedBlocks, getDeadEndBlocks());
-                       })) {
+      if (llvm::any_of(borrowScopeIntroducers, [&](BorrowedValue borrowScope) {
+            return !borrowScope.areUsesWithinScope(
+                phiArgLR.getAllConsumingUses(), scratchSpace, visitedBlocks,
+                getDeadEndBlocks());
+          })) {
         return false;
       }
     }
@@ -1641,7 +1682,7 @@ class StorageGuaranteesLoadVisitor
     SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
     LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());
     if (!checker.validateLifetime(access, endScopeUses,
-                                  liveRange.getDestroyingUses())) {
+                                  liveRange.getAllConsumingUses())) {
       // If we fail the linear lifetime check, then just recur:
       return next(access->getOperand());
     }
@@ -1738,8 +1779,8 @@ class StorageGuaranteesLoadVisitor
     LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());
 
     // Returns true on success. So we invert.
-    bool foundError = !checker.validateLifetime(baseObject, endScopeInsts,
-                                                liveRange.getDestroyingUses());
+    bool foundError = !checker.validateLifetime(
+        baseObject, endScopeInsts, liveRange.getAllConsumingUses());
     return answer(foundError);
   }
 
@@ -1777,7 +1818,7 @@ class StorageGuaranteesLoadVisitor
     // Returns true on success. So we invert.
     bool foundError = !checker.validateLifetime(
         stack, destroyAddrOperands /*consuming users*/,
-        liveRange.getDestroyingUses() /*non consuming users*/);
+        liveRange.getAllConsumingUses() /*non consuming users*/);
     return answer(foundError);
   }
 

test/SILOptimizer/semantic-arc-opts.sil

@@ -59,6 +59,7 @@ class ClassLet {
   @_hasStorage let aLet: Klass
   @_hasStorage var aVar: Klass
   @_hasStorage let aLetTuple: (Klass, Klass)
+  @_hasStorage let anOptionalLet: FakeOptional<Klass>
 
   @_hasStorage let anotherLet: ClassLet
 }
@@ -2204,3 +2205,31 @@ bb6:
   %9999 = tuple()
   return %9999 : $()
 }
+
+// Make sure that we do not promote the load [copy] to a load_borrow since it
+// has a use outside of the access scope.
+//
+// CHECK-LABEL: sil [ossa] @deadEndBlockDoNotPromote : $@convention(method) (@guaranteed ClassLet) -> () {
+// CHECK: load_borrow
+// CHECK: load [copy]
+// CHECK: } // end sil function 'deadEndBlockDoNotPromote'
+sil [ossa] @deadEndBlockDoNotPromote : $@convention(method) (@guaranteed ClassLet) -> () {
+bb0(%0 : @guaranteed $ClassLet):
+  %4 = ref_element_addr %0 : $ClassLet, #ClassLet.anotherLet
+  %5 = load [copy] %4 : $*ClassLet
+  %6 = begin_borrow %5 : $ClassLet
+  %7 = ref_element_addr %6 : $ClassLet, #ClassLet.anOptionalLet
+  %8 = begin_access [read] [dynamic] %7 : $*FakeOptional<Klass>
+  %9 = load [copy] %8 : $*FakeOptional<Klass>
+  end_access %8 : $*FakeOptional<Klass>
+  end_borrow %6 : $ClassLet
+  destroy_value %5 : $ClassLet
+  switch_enum %9 : $FakeOptional<Klass>, case #FakeOptional.none!enumelt: bb1, case #FakeOptional.some!enumelt: bb2
+
+bb1:
+  %107 = tuple ()
+  return %107 : $()
+
+bb2(%39 : @owned $Klass):
+  unreachable
+}