Simplify AccessEnforcementOpts by leaving access markers in place.

The extra cost of deterministically deleting instructions is unnecessary. In the
long term, we'll want to verify that access markers exist after all SIL
passes. So just change their enforcement level rather than removing them.

Author: atrick

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

@@ -19,9 +19,10 @@
 ///
 /// This pass optimizes access enforcement as follows:
 ///
-/// **Access marker folding**: Find begin/end access scopes that are
-/// uninterrupted by a potential conflicting access. Flag those as [nontracking]
-/// access.
+/// **Access marker folding**
+///
+/// Find begin/end access scopes that are uninterrupted by a potential
+/// conflicting access. Flag those as [nontracking] access.
 ///
 /// Folding must prove that no dynamic conflicts occur inside of an access
 /// scope. That is, a scope has no "nested inner conflicts". The access itself
@@ -50,12 +51,12 @@
 /// **Local access marker removal**
 ///
 /// When none of the local accesses on local storage (box/stack) have nested
-/// conflicts, then all the locall accesses may be removed. This is somwhat rare
-/// because static diagnostics already promote the obvious cases to static
-/// checks. However, there are two reasons that dynamic local markers may be
-/// removed: (1) inlining may cause closure access to become local access (2)
-/// local storage may truly escape, but none of the the local access scopes
-/// cross a call site.
+/// conflicts, then all the local accesses may be disabled by setting their
+/// enforcement to `static`. This is somwhat rare because static diagnostics
+/// already promote the obvious cases to static checks. However, there are two
+/// reasons that dynamic local markers may be disabled: (1) inlining may cause
+/// closure access to become local access (2) local storage may truly escape,
+/// but none of the the local access scopes cross a call site.
 ///
 /// TODO: Perform another run of AccessEnforcementSelection immediately before
 /// this pass. Currently, that pass only works well when run before
@@ -575,7 +576,7 @@ foldNonNestedAccesses(AccessConflictAnalysis::AccessMap &accessMap) {
 
 /// Perform local access marker elimination.
 ///
-/// Eliminate accesses to uniquely identified local storage for which no
+/// Disable accesses to uniquely identified local storage for which no
 /// accesses can have nested conflicts. This is only valid if the function's
 /// local storage cannot be potentially modified by unidentified access:
 ///
@@ -590,11 +591,8 @@ foldNonNestedAccesses(AccessConflictAnalysis::AccessMap &accessMap) {
 ///   enforcement). These accesses can only be eliminated when there is no
 ///   Unidentified access within the function without the [no_nested_conflict]
 ///   flag.
-///
-/// This simply invalidates the AccessMap result rather than erasing individual
-/// entries.
 static bool
-removeLocalNonNestedAccess(AccessConflictAnalysis::Result &&result,
+removeLocalNonNestedAccess(const AccessConflictAnalysis::Result &result,
                            const FunctionAccessedStorage &functionAccess) {
   if (functionAccess.hasUnidentifiedAccess())
     return false;
@@ -603,24 +601,18 @@ removeLocalNonNestedAccess(AccessConflictAnalysis::Result &&result,
   SmallVector<BeginAccessInst *, 8> deadAccesses;
   for (auto &beginAccessAndInfo : result.accessMap) {
     BeginAccessInst *beginAccess = beginAccessAndInfo.first;
-    AccessInfo &info = beginAccessAndInfo.second;
+    const AccessInfo &info = beginAccessAndInfo.second;
     if (info.seenNestedConflict() || !info.isLocal())
       continue;
 
     // This particular access to local storage is marked
     // [no_nested_conflict]. Now check FunctionAccessedStorage to determine if
     // that is true for all access to the same storage.
-    if (functionAccess.hasNoNestedConflict(info))
-      deadAccesses.push_back(beginAccess);
-  }
-  std::sort(deadAccesses.begin(), deadAccesses.end(),
-            [&result](BeginAccessInst *a, BeginAccessInst *b) {
-              return result.getAccessIndex(a) < result.getAccessIndex(b);
-            });
-  for (BeginAccessInst *beginAccess : deadAccesses) {
-    DEBUG(llvm::dbgs() << "Removing dead access " << *beginAccess);
-    changed = true;
-    removeBeginAccess(beginAccess);
+    if (functionAccess.hasNoNestedConflict(info)) {
+      DEBUG(llvm::dbgs() << "Disabling dead access " << *beginAccess);
+      beginAccess->setEnforcement(SILAccessEnforcement::Static);
+      changed = true;
+    }
   }
   return changed;
 }
@@ -647,15 +639,14 @@ struct AccessEnforcementOpts : public SILFunctionTransform {
 
     // Use the updated AccessedStorageAnalysis to find any uniquely identified
     // local storage that has no nested conflict on any of its accesses within
-    // this function. These can be removed entirely.
+    // this function. All the accesses can be marked as statically enforced.
     //
     // Note that the storage address may be passed as an argument and there may
     // be nested conflicts within that call, but none of the accesses within
     // this function will overlap.
     const FunctionAccessedStorage &functionAccess = ASA->getEffects(F);
-    if (removeLocalNonNestedAccess(std::move(result), functionAccess))
+    if (removeLocalNonNestedAccess(result, functionAccess))
       invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
-    // `result` is now invalid.
   }
 };
 } // namespace

test/SILOptimizer/access_enforcement_opts.sil

@@ -197,7 +197,7 @@ bb0:
 // Demote both read accesses to non_nested, then remove the local outer access.
 //
 // CHECK-LABEL: sil @testCaptureReadRead : $@convention(thin) () -> () {
-// CHECK-NOT: begin_access
+// CHECK: begin_access [read] [static] [no_nested_conflict] %0 : $*X
 // CHECK-LABEL: } // end sil function 'testCaptureReadRead'
 sil @testCaptureReadRead : $@convention(thin) () -> () {
 bb0:
@@ -246,7 +246,9 @@ bb0(%0 : $*X):
 // Demote both read accesses to non_nested, and remove the local outer access.
 //
 // CHECK-LABEL: sil @testCaptureBoxReadRead : $@convention(thin) () -> () {
-// CHECK-NOT: begin_access
+// CHECK: [[BOX:%.*]] = alloc_box ${ var X }, var, name "x"
+// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var X }, 0
+// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*X
 // CHECK-LABEL: } // end sil function 'testCaptureBoxReadRead'
 sil @testCaptureBoxReadRead : $@convention(thin) () -> () {
 bb0:
@@ -361,7 +363,10 @@ bb0(%0 : $*Int64):
 // Demote read/read access to [no_nested_conflict].
 //
 // CHECK-LABEL: sil @testInoutReadEscapeRead : $@convention(thin) () -> () {
-// CHECK-NOT: begin_access
+// CHECK: [[BOX:%.*]] = alloc_box ${ var Int64 }, var, name "x"
+// CHECK: [[BOXADR:%.*]] = project_box [[BOX]] : ${ var Int64 }, 0
+// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*Int64
+// CHECK: begin_access [read] [static] [no_nested_conflict] [[BOXADR]] : $*Int64
 // CHECK-LABEL: } // end sil function 'testInoutReadEscapeRead'
 sil @testInoutReadEscapeRead : $@convention(thin) () -> () {
 bb0: