[LICM/Exclusivity] Hoist (some) conflicting begin_accesses out of loops

Consider the attached test cases:

We have a begin_access [dynamic] to a global inside of a loop

There’s a nested conflict on said access due to an apply() instruction between the begin and end accesses.

LICM is currently very conservative: If there are any function calls inside of the loop that conflict with begin and end access, we do not hoist out of the loop.

However, if all conflicting applies are “sandwiched” between the begin and end access. So there’s no reason we can’t hoist out of the loop.

See radar rdar://problem/43660965 - this improves some internal benchmarks by over 3X

Author: Joe Shajrawi

lib/SILOptimizer/LoopTransforms/LICM.cpp

@@ -537,7 +537,8 @@ static bool handledEndAccesses(BeginAccessInst *BI, SILLoop *Loop) {
 static bool analyzeBeginAccess(BeginAccessInst *BI,
                                SmallVector<BeginAccessInst *, 8> &BeginAccesses,
                                SmallVector<FullApplySite, 8> &fullApplies,
-                               AccessedStorageAnalysis *ASA) {
+                               AccessedStorageAnalysis *ASA,
+                               DominanceInfo *DT) {
   if (BI->getEnforcement() != SILAccessEnforcement::Dynamic) {
     return false;
   }
@@ -564,8 +565,18 @@ static bool analyzeBeginAccess(BeginAccessInst *BI,
     FunctionAccessedStorage callSiteAccesses;
     ASA->getCallSiteEffects(callSiteAccesses, fullApply);
     SILAccessKind accessKind = BI->getAccessKind();
-    if (callSiteAccesses.mayConflictWith(accessKind, storage))
+    if (!callSiteAccesses.mayConflictWith(accessKind, storage))
+      continue;
+    // Check if we can ignore this conflict:
+    // If the apply is “sandwiched” between the begin and end access,
+    // there’s no reason we can’t hoist out of the loop.
+    auto *applyInstr = fullApply.getInstruction();
+    if (!DT->dominates(BI, applyInstr))
       return false;
+    for (auto *EI : BI->getEndAccesses()) {
+      if (!DT->dominates(applyInstr, EI))
+        return false;
+    }
   }
 
   return true;
@@ -687,7 +698,7 @@ void LoopTreeOptimization::analyzeCurrentLoop(
       LLVM_DEBUG(llvm::dbgs() << "Some end accesses can't be handled\n");
       continue;
     }
-    if (analyzeBeginAccess(BI, BeginAccesses, fullApplies, ASA)) {
+    if (analyzeBeginAccess(BI, BeginAccesses, fullApplies, ASA, DomTree)) {
       SpecialHoist.insert(BI);
     }
   }

test/SILOptimizer/licm_exclusivity.sil

@@ -0,0 +1,80 @@
+// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=checked -enable-sil-verify-all %s -licm | %FileCheck %s
+
+sil_stage canonical
+
+
+import Builtin
+import Swift
+
+struct X {
+  @sil_stored var i: Int64 { get set }
+  init(i: Int64)
+  init()
+}
+
+var globalX: X
+
+sil_global hidden @globalX : $X
+
+
+sil hidden_external [global_init] @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
+
+// public func hoist_access_with_conflict() {
+// Tests Hoisting of begin/end access when there's a "sandwiched" unidentified access
+//
+// CHECK-LABEL: sil @hoist_access_with_conflict : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK: [[BEGIN:%.*]] = begin_access [read] [dynamic] %0 : $*X
+// CHECK-NEXT: br bb1
+// CHECK: apply
+// CHECK: load
+// CHECK: cond_br
+// CHECK: bb2
+// CHECK: end_access [[BEGIN]]
+// CHECK-LABEL: } // end sil function 'hoist_access_with_conflict'
+sil @hoist_access_with_conflict : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
+  br bb1
+  
+bb1:
+  %u3 = begin_access [read] [dynamic] %0 : $*X
+  %u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
+  %u4 = load %u3 : $*X
+  end_access %u3 : $*X
+  cond_br undef, bb1, bb2
+
+bb2:
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// public func dont_hoist_access_with_conflict() {
+// Tests *not* hoisting begin/end access when there's an unidentified access not protected by them
+//
+// CHECK-LABEL: sil @dont_hoist_access_with_conflict : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK: br bb1
+// CHECK: apply
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [read] [dynamic] %0 : $*X
+// CHECK-NEXT: load
+// CHECK-NEXT: end_access [[BEGIN]]
+// CHECK-LABEL: } // end sil function 'dont_hoist_access_with_conflict'
+sil @dont_hoist_access_with_conflict : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
+  br bb1
+  
+bb1:
+  %u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
+  %u3 = begin_access [read] [dynamic] %0 : $*X
+  %u4 = load %u3 : $*X
+  end_access %u3 : $*X
+  cond_br undef, bb1, bb2
+
+bb2:
+  %10 = tuple ()
+  return %10 : $()
+}