[semantic-arc-opts] Use the LiveRange abstraction to find destroys instead of iterating over direct uses so we handle forwarding uses.

Previously, we were incorrectly handling these test cases since we weren't
properly finding destroys through forwarding instructions. I fixed that in a
previous commit by changing the code here to only support load [copy] without
forwarding instructions.

In this commit, I change the code to instead use the LiveRange abstraction. The
LiveRange abstraction already knows how to find destroys through forwarding
instructions and has this destroy array already computed for us!

So we get better runtime performance of code (due to the better opt) and better
compile time (since we aren't computing the destroy list twice)!

rdar://58289320

Author: gottesmm

lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp

@@ -36,25 +36,6 @@ STATISTIC(NumEliminatedInsts, "number of removed instructions");
 STATISTIC(NumLoadCopyConvertedToLoadBorrow,
           "number of load_copy converted to load_borrow");
 
-//===----------------------------------------------------------------------===//
-//                                  Utility
-//===----------------------------------------------------------------------===//
-
-static bool isConsumingOwnedUse(Operand *use) {
-  assert(use->get().getOwnershipKind() == ValueOwnershipKind::Owned);
-
-  if (use->isTypeDependent())
-    return false;
-
-  // We know that a copy_value produces an @owned value. Look through all of
-  // our uses and classify them as either invalidating or not
-  // invalidating. Make sure that all of the invalidating ones are
-  // destroy_value since otherwise the live_range is not complete.
-  auto map = use->getOwnershipKindMap();
-  auto constraint = map.getLifetimeConstraint(ValueOwnershipKind::Owned);
-  return constraint == UseLifetimeConstraint::MustBeInvalidated;
-}
-
 //===----------------------------------------------------------------------===//
 //                            Live Range Modeling
 //===----------------------------------------------------------------------===//
@@ -325,7 +306,7 @@ struct SemanticARCOptVisitor
   FORWARDING_TERM(CondBranch)
 #undef FORWARDING_TERM
 
-  bool isWrittenTo(LoadInst *li, ArrayRef<SILInstruction *> destroys);
+  bool isWrittenTo(LoadInst *li, const LiveRange &lr);
 
   bool processWorklist();
 
@@ -695,22 +676,20 @@ class StorageGuaranteesLoadVisitor
 {
   // The outer SemanticARCOptVisitor.
   SemanticARCOptVisitor &ARCOpt;
-  
-  // The original load instruction.
-  LoadInst *Load;
-  
+
+  // The live range of the original load.
+  const LiveRange &liveRange;
+
   // The current address being visited.
   SILValue currentAddress;
 
   Optional<bool> isWritten;
 
-  ArrayRef<SILInstruction *> destroyValues;
-
 public:
   StorageGuaranteesLoadVisitor(SemanticARCOptVisitor &arcOpt, LoadInst *load,
-                               ArrayRef<SILInstruction *> destroyValues)
-      : ARCOpt(arcOpt), Load(load), currentAddress(load->getOperand()),
-        destroyValues(destroyValues) {}
+                               const LiveRange &liveRange)
+      : ARCOpt(arcOpt), liveRange(liveRange),
+        currentAddress(load->getOperand()) {}
 
   void answer(bool written) {
     currentAddress = nullptr;
@@ -793,28 +772,11 @@ class StorageGuaranteesLoadVisitor
     llvm::copy(borrowInst->getUsersOfType<EndBorrowInst>(),
                std::back_inserter(baseEndBorrows));
 
-    SmallVector<SILInstruction *, 4> valueDestroys;
-    for (auto *use : Load->getUses()) {
-      // If this load is not consuming, skip it.
-      if (!isConsumingOwnedUse(use)) {
-        continue;
-      }
-
-      // Otherwise, if this isn't a destroy_value, we have a consuming use we
-      // don't understand. Return conservatively that this memory location may
-      // not be guaranteed.
-      auto *user = use->getUser();
-      if (!isa<DestroyValueInst>(user)) {
-        return answer(true);
-      }
-      valueDestroys.emplace_back(user);
-    }
-
     SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
     LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());
     // Returns true on success. So we invert.
-    bool foundError =
-        !checker.validateLifetime(baseObject, baseEndBorrows, valueDestroys);
+    bool foundError = !checker.validateLifetime(baseObject, baseEndBorrows,
+                                                liveRange.getDestroys());
     return answer(foundError);
   }
 
@@ -850,9 +812,9 @@ class StorageGuaranteesLoadVisitor
     SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
     LinearLifetimeChecker checker(visitedBlocks, ARCOpt.getDeadEndBlocks());
     // Returns true on success. So we invert.
-    bool foundError =
-        !checker.validateLifetime(stack, destroyAddrs /*consuming users*/,
-                                  destroyValues /*non consuming users*/);
+    bool foundError = !checker.validateLifetime(
+        stack, destroyAddrs /*consuming users*/,
+        liveRange.getDestroys() /*non consuming users*/);
     return answer(foundError);
   }
 
@@ -866,9 +828,8 @@ class StorageGuaranteesLoadVisitor
 
 } // namespace
 
-bool SemanticARCOptVisitor::isWrittenTo(LoadInst *load,
-                                        ArrayRef<SILInstruction *> destroys) {
-  StorageGuaranteesLoadVisitor visitor(*this, load, destroys);
+bool SemanticARCOptVisitor::isWrittenTo(LoadInst *load, const LiveRange &lr) {
+  StorageGuaranteesLoadVisitor visitor(*this, load, lr);
   return visitor.doIt();
 }
 
@@ -892,8 +853,7 @@ bool SemanticARCOptVisitor::visitLoadInst(LoadInst *li) {
   // Then check if our address is ever written to. If it is, then we cannot use
   // the load_borrow because the stored value may be released during the loaded
   // value's live range.
-  auto destroyValues = lr.getDestroys();
-  if (isWrittenTo(li, destroyValues))
+  if (isWrittenTo(li, lr))
     return false;
 
   // Ok, we can perform our optimization. Convert the load [copy] into a
@@ -906,6 +866,7 @@ bool SemanticARCOptVisitor::visitLoadInst(LoadInst *li) {
   // to find the post-dominating block set of these destroy value to ensure that
   // we do not insert multiple end_borrow.
   assert(lifetimeFrontier.empty());
+  auto destroyValues = lr.getDestroys();
   ValueLifetimeAnalysis analysis(li, destroyValues);
   bool foundCriticalEdges = !analysis.computeFrontier(
       lifetimeFrontier, ValueLifetimeAnalysis::DontModifyCFG,

test/SILOptimizer/semantic-arc-opts.sil

@@ -589,11 +589,8 @@ bb0(%x : @owned $ClassLet):
   return undef : $()
 }
 
-// We do not support this today, but we will once forwarding is ignored when
-// checking if the load [copy] is a dead live range.
-//
 // CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase :
-// CHECK: load [copy]
+// CHECK: load_borrow
 // CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase'
 sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase : $@convention(thin) (@owned ClassLet) -> () {
 bb0(%x : @owned $ClassLet):
@@ -613,11 +610,8 @@ bb0(%x : @owned $ClassLet):
   return undef : $()
 }
 
-// We do not support this today, but we will once forwarding is ignored when
-// checking if the load [copy] is a dead live range.
-//
 // CHECK-LABEL: sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 :
-// CHECK: load [copy]
+// CHECK: load_borrow
 // CHECK: } // end sil function 'dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2'
 sil [ossa] @dont_copy_let_properties_with_borrowed_base_that_dominates_projtestcase_2 : $@convention(thin) (@owned ClassLet) -> () {
 bb0(%x : @owned $ClassLet):