DominatedAccessRemoval: fix a bug wherein we incorrectly removes nested conflicting accesses

radar rdar://problem/48323041

Author: Joe Shajrawi

lib/SILOptimizer/Transforms/AccessEnforcementDom.cpp

@@ -145,6 +145,17 @@ bool DominatedAccessRemoval::visitInstruction(
   return true;
 }
 
+static EndAccessInst *getSingleEndAccess(BeginAccessInst *inst) {
+  EndAccessInst *end = nullptr;
+  for (auto *currEnd : inst->getEndAccesses()) {
+    if (end == nullptr)
+      end = currEnd;
+    else
+      return nullptr;
+  }
+  return end;
+}
+
 void DominatedAccessRemoval::visitBeginAccess(
     BeginAccessInst *beginAccess, AccessedStorage storage,
     AccessedStorageInfo &visitedDomAccessesToStorageInfo) {
@@ -185,6 +196,20 @@ void DominatedAccessRemoval::visitBeginAccess(
   }
 
   auto *parentBegin = visitedAccessesIt->first;
+
+  // If this beginAccess is a modify, and is nested inside parent
+  // then we have to bail: can't change it to static
+  // If, on the other hand, it is a read, we can continue.
+  if (beginAccess->getAccessKind() >= SILAccessKind::Modify) {
+    auto *endP = getSingleEndAccess(parentBegin);
+    if (!endP) {
+      return;
+    }
+    if (!domInfo->properlyDominates(endP, beginAccess)) {
+      return;
+    }
+  }
+
   if (beginAccess->getAccessKind() > parentBegin->getAccessKind()) {
     // we can't change to static: dominating access has a > access kind
     // change parent's access kind so we would be able to do so

test/SILOptimizer/access_dom.sil

@@ -540,3 +540,47 @@ bb4:
   %10 = tuple ()
   return %10 : $()
 }
+
+// public func testBailOnNestedDomWrite() {
+// Checks that we bail when the dominated begin comes before the dominating begin
+//
+// CHECK-LABEL: sil @testBailOnNestedDomWrite : $@convention(thin) () -> () {
+// CHECK: bb0:
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [no_nested_conflict] [[GLOBAL]] : $*X
+// CHECK-LABEL: } // end sil function 'testBailOnNestedDomWrite'
+sil @testBailOnNestedDomWrite : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %4 = begin_access [modify] [dynamic] %0 : $*X
+  %5 = load %4 : $*X
+  %7 = begin_access [modify] [dynamic] [no_nested_conflict] %0 : $*X
+  %8 = load %7 : $*X
+  end_access %7 : $*X
+  end_access %4 : $*X
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// public func testOptOnNestedDomRead() {
+// Checks that we bail when the dominated begin comes before the dominating begin
+//
+// CHECK-LABEL: sil @testOptOnNestedDomRead : $@convention(thin) () -> () {
+// CHECK: bb0:
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK: [[BEGIN:%.*]] = begin_access [read] [dynamic] [[GLOBAL]] : $*X
+// CHECK: [[BEGIN2:%.*]] = begin_access [read] [static] [no_nested_conflict] [[GLOBAL]] : $*X
+// CHECK-LABEL: } // end sil function 'testOptOnNestedDomRead'
+sil @testOptOnNestedDomRead : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %4 = begin_access [read] [dynamic] %0 : $*X
+  %5 = load %4 : $*X
+  %7 = begin_access [read] [dynamic] [no_nested_conflict] %0 : $*X
+  %8 = load %7 : $*X
+  end_access %7 : $*X
+  end_access %4 : $*X
+  %10 = tuple ()
+  return %10 : $()
+}