[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 VisitBarrierAccessScopes::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,18 +145,29 @@ class Dataflow final {
 
 private:
   friend Reachability;
+  friend class BarrierAccessScopeFinder;
+  friend class VisitBarrierAccessScopes<Dataflow, BarrierAccessScopeFinder>;
 
   Classification classifyInstruction(SILInstruction *);
 
   bool classificationIsBarrier(Classification);
 
   /// IterativeBackwardReachability::Effects
+  /// VisitBarrierAccessScopes::Effects
 
   ArrayRef<SILInstruction *> gens() { return uses.ends; }
 
   Effect effectForInstruction(SILInstruction *);
   Effect effectForPhi(SILBasicBlock *);
 
+  /// VisitBarrierAccessScopes::Effects
+
+  auto localGens() { return result.localGens; }
+
+  bool isLocalGen(SILInstruction *instruction) {
+    return result.localGens.contains(instruction);
+  }
+
   /// IterativeBackwardReachability::bindBarriers::Visitor
 
   void visitBarrierInstruction(SILInstruction *instruction) {
@@ -173,6 +189,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;
   }
@@ -209,8 +230,41 @@ Dataflow::Effect Dataflow::effectForPhi(SILBasicBlock *block) {
   return isBarrier ? Effect::Kill() : Effect::NoEffect();
 }
 
+/// 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>;
+  Impl impl;
+  Dataflow &dataflow;
+
+public:
+  BarrierAccessScopeFinder(Context const &context, Dataflow &dataflow)
+      : impl(&context.function, dataflow, *this), dataflow(dataflow) {}
+
+  void find() { impl.visit(); }
+
+private:
+  friend Impl;
+
+  bool isInRegion(SILBasicBlock *block) {
+    return dataflow.result.discoveredBlocks.contains(block);
+  }
+
+  void visitBarrierAccessScope(BeginAccessInst *bai) {
+    dataflow.barrierAccessScopes.insert(bai);
+    for (auto *eai : bai->getEndAccesses()) {
+      dataflow.reachability.addKill(eai);
+    }
+  }
+};
+
 void Dataflow::run() {
   reachability.initialize();
+  BarrierAccessScopeFinder finder(context, *this);
+  finder.find();
   reachability.solve();
   reachability.findBarriers(*this);
 }

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                                                         }}
+// =============================================================================