[LexicalDestroyHoisting] Adopt VisitBarrierAccessScopes.

Avoids hoisting destroy_values into unrelated access scopes which could
introduce exclusivity violations.

Author: nate-chandler

lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp

@@ -20,6 +20,7 @@
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/SILValue.h"
 #include "swift/SILOptimizer/Analysis/Reachability.h"
+#include "swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h"
 #include "swift/SILOptimizer/Utils/CanonicalizeBorrowScope.h"
 #include "swift/SILOptimizer/Utils/InstOptUtils.h"
 #include "swift/SILOptimizer/Utils/InstructionDeleter.h"
@@ -112,11 +113,14 @@ struct DeinitBarriers final {
   DeinitBarriers &operator=(DeinitBarriers const &) = delete;
 };
 
+class BarrierAccessScopeFinder;
+
 /// Works backwards from the current location of destroy_values to the earliest
 /// place they can be hoisted to.
 ///
 /// Implements IterativeBackwardReachability::Effects
 /// Implements IterativeBackwardReachability::bindBarriers::Visitor
+/// Implements BarrierAccessScopeFinder::Effects
 class Dataflow final {
   using Reachability = IterativeBackwardReachability<Dataflow>;
   using Effect = Reachability::Effect;
@@ -125,6 +129,7 @@ class Dataflow final {
   DeinitBarriers &barriers;
   Reachability::Result result;
   Reachability reachability;
+  SmallPtrSet<BeginAccessInst *, 8> barrierAccessScopes;
 
   enum class Classification { Barrier, Other };
 
@@ -140,6 +145,8 @@ class Dataflow final {
 
 private:
   friend Reachability;
+  friend class BarrierAccessScopeFinder;
+  friend class VisitBarrierAccessScopes<Dataflow, BarrierAccessScopeFinder>;
 
   Classification classifyInstruction(SILInstruction *);
 
@@ -154,8 +161,37 @@ class Dataflow final {
   void visitBarrierBlock(SILBasicBlock *);
 };
 
+/// Finds end_access instructions which are barriers to hoisting because the
+/// access scopes they contain barriers to hoisting.  Hoisting destroy_values
+/// into such access scopes could introduce exclusivity violations.
+///
+/// Implements BarrierAccessScopeFinder::Visitor
+class BarrierAccessScopeFinder final {
+  using Impl = VisitBarrierAccessScopes<Dataflow, BarrierAccessScopeFinder>;
+  Context const &context;
+  Impl impl;
+  Dataflow &dataflow;
+  Optional<SmallVector<SILBasicBlock *, 16>> cachedRoots;
+
+public:
+  BarrierAccessScopeFinder(Context const &context, Dataflow &dataflow)
+      : context(context), impl(&context.function, dataflow, *this),
+        dataflow(dataflow) {}
+
+  void find();
+
+private:
+  friend Impl;
+
+  ArrayRef<SILBasicBlock *> roots();
+  bool isInRegion(SILBasicBlock *);
+  void visitBarrierAccessScope(BeginAccessInst *);
+};
+
 void Dataflow::run() {
   reachability.initialize();
+  BarrierAccessScopeFinder finder(context, *this);
+  finder.find();
   reachability.solve();
   reachability.findBarriers(*this);
 }
@@ -168,6 +204,11 @@ Dataflow::classifyInstruction(SILInstruction *instruction) {
   if (uses.users.contains(instruction)) {
     return Classification::Barrier;
   }
+  if (auto *eai = dyn_cast<EndAccessInst>(instruction)) {
+    return barrierAccessScopes.contains(eai->getBeginAccess())
+               ? Classification::Barrier
+               : Classification::Other;
+  }
   if (isDeinitBarrier(instruction)) {
     return Classification::Barrier;
   }
@@ -218,6 +259,33 @@ void Dataflow::visitBarrierBlock(SILBasicBlock *block) {
   barriers.blocks.push_back(block);
 }
 
+void BarrierAccessScopeFinder::find() { impl.visit(); }
+
+ArrayRef<SILBasicBlock *> BarrierAccessScopeFinder::roots() {
+  if (cachedRoots)
+    return *cachedRoots;
+
+  cachedRoots = SmallVector<SILBasicBlock *, 16>{};
+  BasicBlockSet seenRoots(&context.function);
+  for (auto *gen : dataflow.gens()) {
+    seenRoots.insert(gen->getParent());
+    cachedRoots->push_back(gen->getParent());
+  }
+
+  return *cachedRoots;
+}
+
+bool BarrierAccessScopeFinder::isInRegion(SILBasicBlock *block) {
+  return dataflow.result.discoveredBlocks.contains(block);
+}
+
+void BarrierAccessScopeFinder::visitBarrierAccessScope(BeginAccessInst *bai) {
+  dataflow.barrierAccessScopes.insert(bai);
+  for (auto *eai : bai->getEndAccesses()) {
+    dataflow.reachability.addKill(eai);
+  }
+}
+
 /// Hoist the destroy_values of %value.
 class Rewriter final {
   Context &context;

test/SILOptimizer/lexical_destroy_hoisting.sil

@@ -460,3 +460,67 @@ entry(%instance : @owned $C, %input : $S):
 // instruction tests                                                          }}
 // =============================================================================
 
+// =============================================================================
+// access scope tests                                                         {{
+// =============================================================================
+
+// Don't hoist into an access scope that contains a barrier.
+//
+// CHECK-LABEL: sil [ossa] @nofold_scoped_load_barrier : {{.*}} {
+// CHECK: end_access
+// CHECK: end_access
+// CHECK: destroy_value
+// CHECK-LABEL: // end sil function 'nofold_scoped_load_barrier'
+sil [ossa] @nofold_scoped_load_barrier : $@convention(thin) (@owned C, @owned C) -> (@owned C) {
+entry(%instance : @owned $C, %other : @owned $C):
+  %addr = alloc_stack $C
+  %store_scope = begin_access [modify] [static] %addr : $*C
+  store %other to [init] %store_scope : $*C
+  end_access %store_scope : $*C
+  %load_scope = begin_access [read] [static] %addr : $*C
+  %value = load [copy] %load_scope : $*C
+  %barrier = function_ref @barrier : $@convention(thin) () -> ()
+  apply %barrier() : $@convention(thin) () -> ()
+  end_access %load_scope : $*C
+  destroy_addr %addr : $*C
+  dealloc_stack %addr : $*C
+  destroy_value %instance : $C
+  return %value : $C
+}
+
+// Access scopes that are open at barrier blocks are barriers.  Otherwise, we
+// would hoist destroy_values into the scopes when the destroy_values are
+// hoisted up to the begin of blocks whose predecessor is the barrier block.
+//
+// CHECK-LABEL: sil [ossa] @nohoist_into_access_scope_barred_by_barrier_block : {{.*}} {
+// CHECK:       {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $C, [[INOUT:%[^,]+]] : $*C):
+// CHECK:         [[SCOPE:%[^,]+]] = begin_access [modify] [static] [[INOUT]] : $*C
+// CHECK:         cond_br undef, [[LEFT:bb[0-9]+]],
+// CHECK:       [[LEFT]]:
+// CHECK:         end_access [[SCOPE]] : $*C
+// CHECK-NEXT:    destroy_value [[INSTANCE]] : $C
+// CHECK-LABEL: } // end sil function 'nohoist_into_access_scope_barred_by_barrier_block'
+sil [ossa] @nohoist_into_access_scope_barred_by_barrier_block : $@convention(thin) (@owned C, @inout C) -> () {
+entry(%instance : @owned $C, %second : $*C):
+    %scope = begin_access [modify] [static] %second : $*C
+    cond_br undef, left, right
+
+left:
+    end_access %scope : $*C
+    %ignore = tuple ()
+    destroy_value %instance : $C
+    br exit
+
+right:
+    end_access %scope : $*C
+    apply undef(%instance) : $@convention(thin) (@owned C) -> ()
+    br exit
+
+exit:
+    %retval = tuple ()
+    return %retval : $()
+}
+
+// =============================================================================
+// access scope tests                                                         }}
+// =============================================================================