Add AccessEnforcementOpts fast paths.

1. During identifyAccess, determine if there are either any
identical accesses or an accesses that aren't already marked
no_nested_storage. If there are neither, then skip the subsequent
data flow analysis.

2. In the new StorageSet, indicate whether identical storage was
seen elsewhere in the function. During dataflow, only add an access
to the out-of-scope access set if was marked as having identical
storage with another access.

3. During data flow, don't track in scope conflicts for
instructions already marked [no_nested_conflict].

Author: atrick

include/swift/SIL/MemAccessUtils.h

@@ -143,7 +143,8 @@ class AccessedStorage {
     SWIFT_INLINE_BITFIELD_FULL(AccessEnforcementOptsInfo, AccessedStorage,
                                64 - NumAccessedStorageBits,
                                seenNestedConflict : 1,
-                               beginAccessIndex : 63 - NumAccessedStorageBits);
+                               seenIdenticalStorage : 1,
+                               beginAccessIndex : 62 - NumAccessedStorageBits);
 
     // Define data flow bits for use in the AccessEnforcementDom pass. Each
     // begin_access in the function is mapped to one instance of this subclass.

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

@@ -129,10 +129,23 @@ class AccessEnforcementOptsInfo : public AccessedStorage {
     Bits.AccessEnforcementOptsInfo.seenNestedConflict = 1;
   }
 
+  /// Did a PostOrder walk previously find another access to the same
+  /// storage. If so, then this access could be merged with a subsequent access
+  /// after checking for conflicts.
+  bool seenIdenticalStorage() const {
+    return Bits.AccessEnforcementOptsInfo.seenIdenticalStorage;
+  }
+
+  void setSeenIdenticalStorage() {
+    Bits.AccessEnforcementOptsInfo.seenIdenticalStorage = 1;
+  }
+
   void dump() const {
     AccessedStorage::dump();
     llvm::dbgs() << "  access index: " << getAccessIndex() << " <"
-                 << (seenNestedConflict() ? "" : "no ") << "conflict>\n";
+                 << (seenNestedConflict() ? "" : "no ") << "conflict> <"
+                 << (seenIdenticalStorage() ? "" : "not ") << "seen identical>"
+                 << "\n";
   }
 };
 using AccessInfo = AccessEnforcementOptsInfo;
@@ -267,21 +280,26 @@ class AccessConflictAndMergeAnalysis {
 
 private:
   LoopRegionFunctionInfo *LRFI;
+  PostOrderFunctionInfo *PO;
   AccessedStorageAnalysis *ASA;
 
+  // Unique storage locations seen in this function.
+  AccessedStorageSet storageSet;
+
   Result result;
 
 public:
   AccessConflictAndMergeAnalysis(LoopRegionFunctionInfo *LRFI,
+                                 PostOrderFunctionInfo *PO,
                                  AccessedStorageAnalysis *ASA)
-      : LRFI(LRFI), ASA(ASA) {}
+    : LRFI(LRFI), PO(PO), ASA(ASA) {}
 
-  void analyze();
+  bool analyze();
 
   const Result &getResult() { return result; }
 
 protected:
-  void identifyBeginAccesses();
+  bool identifyBeginAccesses();
 
   void
   propagateAccessSetsBottomUp(LoopRegionToAccessedStorage &regionToStorageMap,
@@ -456,6 +474,11 @@ void AccessConflictAndMergeAnalysis::insertOutOfScopeAccess(
     RegionState &state, BeginAccessInst *beginAccess,
     AccessInfo &currStorageInfo) {
 
+  if (!currStorageInfo.seenIdenticalStorage()) {
+    LLVM_DEBUG(llvm::dbgs() << "Ignoring unmergeable access: " << *beginAccess);
+    return;
+  }
+
   auto identicalStorageIter = llvm::find_if(
       state.outOfScopeConflictFreeAccesses, [&](BeginAccessInst *bai) {
         auto storageInfo = result.getAccessInfo(bai);
@@ -470,8 +493,13 @@ void AccessConflictAndMergeAnalysis::insertOutOfScopeAccess(
 }
 
 // Top-level driver for AccessConflictAndMergeAnalysis
-void AccessConflictAndMergeAnalysis::analyze() {
-  identifyBeginAccesses();
+//
+// Returns true if the analysis succeeded.
+bool AccessConflictAndMergeAnalysis::analyze() {
+  if (!identifyBeginAccesses()) {
+    LLVM_DEBUG(llvm::dbgs() << "Skipping AccessConflictAndMergeAnalysis...\n");
+    return false;
+  }
   LoopRegionToAccessedStorage accessSetsOfRegions;
   // Populate a worklist of regions such that the top of the worklist is the
   // innermost loop and the bottom of the worklist is the entry block.
@@ -501,6 +529,7 @@ void AccessConflictAndMergeAnalysis::analyze() {
       }
     }
   }
+  return true;
 }
 
 // Find all begin access operations in this function. Map each access to
@@ -509,29 +538,52 @@ void AccessConflictAndMergeAnalysis::analyze() {
 //
 // Also, add the storage location to the function's RegionStorage
 //
+// Returns true if it is worthwhile to continue the analysis.
+//
 // TODO: begin_unpaired_access is not tracked. Even though begin_unpaired_access
 // isn't explicitly paired, it may be possible after devirtualization and
 // inlining to find all uses of the scratch buffer. However, this doesn't
 // currently happen in practice (rdar://40033735).
-void AccessConflictAndMergeAnalysis::identifyBeginAccesses() {
-  for (auto &BB : *LRFI->getFunction()) {
-    for (auto &I : BB) {
+bool AccessConflictAndMergeAnalysis::identifyBeginAccesses() {
+  bool seenPossibleNestedConflict = false;
+  bool seenIdenticalStorage = false;
+  // Scan blocks in PostOrder (bottom-up) to mark any accesses with identical
+  // storage to another reachable access. The earlier access must be marked
+  // because this analysis does forward data flow to find conflicts.
+  for (auto *BB : PO->getPostOrder()) {
+    for (auto &I : llvm::reverse(*BB)) {
       auto *beginAccess = dyn_cast<BeginAccessInst>(&I);
       if (!beginAccess)
         continue;
 
       if (beginAccess->getEnforcement() != SILAccessEnforcement::Dynamic)
         continue;
 
+      if (!beginAccess->hasNoNestedConflict())
+        seenPossibleNestedConflict = true;
+
       // The accessed base is expected to be valid for begin_access, but for
       // now, since this optimization runs at the end of the pipeline, we
       // gracefully ignore unrecognized source address patterns, which show up
       // here as an invalid `storage` value.
-      const AccessedStorage &storage =
+      AccessedStorage storage =
           findAccessedStorageNonNested(beginAccess->getSource());
 
-      auto iterAndSuccess = result.accessMap.try_emplace(
-          beginAccess, static_cast<const AccessInfo &>(storage));
+      auto iterAndInserted = storageSet.insert(storage);
+
+      // After inserting it in storageSet, this storage object can be downcast
+      // to AccessInfo to use the pass-specific bits.
+      auto &accessInfo = static_cast<AccessInfo &>(storage);
+
+      // If the same location was seen later in the CFG, mark this access as one
+      // to check for merging.
+      if (!iterAndInserted.second) {
+        seenIdenticalStorage = true;
+        accessInfo.setSeenIdenticalStorage();
+      }
+
+      auto iterAndSuccess =
+        result.accessMap.try_emplace(beginAccess, accessInfo);
       (void)iterAndSuccess;
       assert(iterAndSuccess.second);
 
@@ -541,6 +593,7 @@ void AccessConflictAndMergeAnalysis::identifyBeginAccesses() {
       assert(!info.seenNestedConflict());
     }
   }
+  return seenPossibleNestedConflict || seenIdenticalStorage;
 }
 
 // Returns a mapping from each loop sub-region to all its access storage
@@ -619,11 +672,14 @@ void AccessConflictAndMergeAnalysis::visitBeginAccess(
   recordInScopeConflicts(state, beginAccessInfo, beginAccess->getAccessKind());
   // Remove in-scope conflicts to avoid checking them again.
   removeConflicts(state.inScopeConflictFreeAccesses, beginAccessInfo);
-  // Always record the current access as in-scope. It can potentially be folded
-  // to [no_nested_conflict] independent of any enclosing access conflicts.
-  bool inserted = state.inScopeConflictFreeAccesses.insert(beginAccess);
-  (void)inserted;
-  assert(inserted && "the begin_access should not have been seen yet.");
+
+  if (!beginAccess->hasNoNestedConflict()) {
+    // Record the current access as in-scope. It can potentially be folded to
+    // [no_nested_conflict] independent of any enclosing access conflicts.
+    bool inserted = state.inScopeConflictFreeAccesses.insert(beginAccess);
+    (void)inserted;
+    assert(inserted && "the begin_access should not have been seen yet.");
+  }
 
   // Find an out-of-scope access that is mergeable with this access. This is
   // done at the BeginAccess because it doesn't matter whether the merged access
@@ -637,7 +693,7 @@ void AccessConflictAndMergeAnalysis::visitBeginAccess(
   if (BeginAccessInst *mergeableAccess =
           findMergeableOutOfScopeAccess(state, beginAccess)) {
     LLVM_DEBUG(llvm::dbgs() << "Found mergable pair: " << *mergeableAccess
-                            << ", " << *beginAccess << "\n");
+                            << "  with " << *beginAccess << "\n");
     result.mergePairs.emplace_back(mergeableAccess, beginAccess);
   }
   // For the purpose of data-flow, removing the out-of-scope access does not
@@ -950,8 +1006,10 @@ static bool mergeAccesses(
     SILFunction *F, PostDominanceInfo *postDomTree,
     const AccessConflictAndMergeAnalysis::MergeablePairs &mergePairs) {
 
-  if (mergePairs.empty())
+  if (mergePairs.empty()) {
+    LLVM_DEBUG(llvm::dbgs() << "Skipping SCC Analysis...\n");
     return false;
+  }
 
   bool changed = false;
 
@@ -999,7 +1057,7 @@ static bool mergeAccesses(
       continue;
 
     LLVM_DEBUG(llvm::dbgs()
-               << "Merging: " << *childIns << " into " << *parentIns << "\n");
+               << "Merging " << *childIns << " into " << *parentIns << "\n");
 
     // Change the no nested conflict of parent if the child has a nested
     // conflict.
@@ -1044,9 +1102,12 @@ struct AccessEnforcementOpts : public SILFunctionTransform {
                             << F->getName() << "\n");
 
     LoopRegionFunctionInfo *LRFI = getAnalysis<LoopRegionAnalysis>()->get(F);
+    PostOrderFunctionInfo *PO = getAnalysis<PostOrderAnalysis>()->get(F);
     AccessedStorageAnalysis *ASA = getAnalysis<AccessedStorageAnalysis>();
-    AccessConflictAndMergeAnalysis a(LRFI, ASA);
-    a.analyze();
+    AccessConflictAndMergeAnalysis a(LRFI, PO, ASA);
+    if (!a.analyze())
+      return;
+
     auto result = a.getResult();
 
     // Perform access folding by setting the [no_nested_conflict] flag on

test/SILOptimizer/access_enforcement_fastpath.sil

@@ -0,0 +1,111 @@
+// RUN: %target-sil-opt -access-enforcement-opts -debug-only=access-enforcement-opts -verify %s -o /dev/null 2>&1 | %FileCheck %s
+//
+// REQUIRES: asserts
+//
+// This tests four kinds of AccessEnforcementOpts fast paths:
+//
+// 1. During identifyAccess, determine if there are either any
+// identical accesses or an accesses that aren't already marked
+// no_nested_storage. If there are neither, then skip the subsequent
+// data flow analysis.
+//
+// 2. In StorageSet, indicate whether identical storage was seen
+// elsewhere in the function. During dataflow, only add an access to
+// the out-of-scope access set if was marked as having identical
+// storage with another access.
+//
+// 3. If no access pairs can be merged, avoid analyzing SCC regions in
+// the control flow graph.
+//
+// 4. During data flow, don't track in scope conflicts for
+// instructions already marked [no_nested_conflict].
+
+sil_stage canonical
+
+import Builtin
+import Swift
+import SwiftShims
+
+public class TestEarlyBail {
+  @_hasStorage var prop1: Int { get set }
+  @_hasStorage var prop2: Int { get set }
+}
+
+// All accesses are no_nested_conflict, and none are identical. Bail.
+// (1) Skipping AccessConflictAndMergeAnalysis...
+//
+// CHECK-LABEL: Running local AccessEnforcementOpts on testEarlyBail
+// CHECK-NOT: Processing
+// CHECK-NOT: No conflict on one path
+// CHECK-NOT: Folding
+// CHECK-NOT: Merging
+// CHECK: Skipping AccessConflictAndMergeAnalysis...
+sil @testEarlyBail : $@convention(method) (@owned TestEarlyBail) -> @owned TestEarlyBail {
+bb0(%0 : $TestEarlyBail):
+  %adr1 = ref_element_addr %0 : $TestEarlyBail, #TestEarlyBail.prop1
+  %a1 = begin_access [modify] [dynamic] [no_nested_conflict] %adr1 : $*Int
+  %v1 = load %a1 : $*Int
+  end_access %a1 : $*Int
+  %adr2 = ref_element_addr %0 : $TestEarlyBail, #TestEarlyBail.prop2
+  %a2 = begin_access [modify] [dynamic] [no_nested_conflict] %adr2 : $*Int
+  %v2 = load %a2 : $*Int
+  end_access %a2 : $*Int
+  return %0 : $TestEarlyBail
+}
+
+// An access is not marked no_nested_conflict. Need to run analysis.
+// NOT (1) Skipping AccessConflictAndMergeAnalysis...
+// (2) Ignoring unique access...
+// (3) Skipping SCC analysis...
+//
+// CHECK-LABEL: Running local AccessEnforcementOpts on testEarlyBailNeedsFolding
+// CHECK: Processing Function: testEarlyBailNeedsFolding
+// CHECK: No conflict on one path from   [[ACCESS:%.*]] = begin_access [modify] [dynamic] %{{.*}} : $*Int
+// CHECK-NEXT:  to   end_access [[ACCESS]] : $*Int
+// CHECK: Ignoring unmergeable access: [[ACCESS]] = begin_access [modify] [dynamic] %{{.*}} : $*Int
+// CHECK: Folding   [[ACCESS]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK: Skipping SCC Analysis...
+sil @testEarlyBailNeedsFolding : $@convention(method) (@owned TestEarlyBail) -> @owned TestEarlyBail {
+bb0(%0 : $TestEarlyBail):
+  %adr1 = ref_element_addr %0 : $TestEarlyBail, #TestEarlyBail.prop1
+  %a1 = begin_access [modify] [dynamic] %adr1 : $*Int
+  %v1 = load %a1 : $*Int
+  end_access %a1 : $*Int
+  return %0 : $TestEarlyBail
+}
+
+// Two accesses have the same storage. Need to run analysis.
+// NOT (1) Skipping AccessConflictAndMergeAnalysis...
+// NOT (2) Ignoring unique access...
+// (4) No Conflict On Path
+// (4) NOT No Conflict On Path
+// NOT (3) Skipping SCC analysis...
+//
+// CHECK-LABEL: Running local AccessEnforcementOpts on testEarlyBailMayMerge
+// CHECK: Processing Function: testEarlyBailMayMerge
+// CHECK: No conflict on one path from   [[ACCESS1:%.*]] = begin_access [modify] [dynamic] %{{.*}} : $*Int
+// CHECK:  to end_access [[ACCESS1]] : $*Int
+// CHECK: Found mergable pair: [[ACCESS1]] = begin_access [modify] [dynamic] %{{.*}} : $*Int
+// CHECK-NEXT: with [[ACCESS2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK-NOT: No conflict on one path from [[ACCESS2]]
+// CHECK: Ignoring unmergeable access: [[ACCESS2]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK-NOT: No conflict on one path from [[ACCESS2]]
+//
+// Note: The order that accesses with no nested conflicts are "Folded" is nondeterministic.
+// CHECK-DAG: Folding [[ACCESS1]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK-DAG: Folding [[ACCESS2]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK: Merging [[ACCESS2]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK-NEXT:  into [[ACCESS1]] = begin_access [modify] [dynamic] [no_nested_conflict] %{{.*}} : $*Int
+// CHECK-NOT: Skipping SCC Analysis...
+sil @testEarlyBailMayMerge : $@convention(method) (@owned TestEarlyBail) -> @owned TestEarlyBail {
+bb0(%0 : $TestEarlyBail):
+  %adr1 = ref_element_addr %0 : $TestEarlyBail, #TestEarlyBail.prop1
+  %a1 = begin_access [modify] [dynamic] %adr1 : $*Int
+  %v1 = load %a1 : $*Int
+  end_access %a1 : $*Int
+  %adr2 = ref_element_addr %0 : $TestEarlyBail, #TestEarlyBail.prop1
+  %a2 = begin_access [modify] [dynamic] [no_nested_conflict] %adr2 : $*Int
+  %v2 = load %a2 : $*Int
+  end_access %a2 : $*Int
+  return %0 : $TestEarlyBail
+}