Merge pull request #29002 from gottesmm/pr-ff454301beb141a7a342948592e5c4436ec3b1e2

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

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):