[Exclusivity] Sink may release release instructions out of access scopes

General case:
begin_access A
...
strong_release / release_value / destroy
end_access

The release instruction can be sunk below the end_access instruction,
This extends the lifetime of the released value, but, might allow us to
Mark the access scope as no nested conflict.

include/swift/SILOptimizer/PassManager/Passes.def

@@ -58,6 +58,8 @@ PASS(AccessEnforcementDom, "access-enforcement-dom",
      "Remove dominated access checks with no nested conflict")
 PASS(AccessEnforcementOpts, "access-enforcement-opts",
      "Access Enforcement Optimization")
+PASS(AccessEnforcementReleaseSinking, "access-enforcement-release",
+     "Access Enforcement Release Sinking")
 PASS(AccessEnforcementSelection, "access-enforcement-selection",
      "Access Enforcement Selection")
 PASS(AccessEnforcementWMO, "access-enforcement-wmo",

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -195,6 +195,7 @@ void addHighLevelLoopOptPasses(SILPassPipelinePlan &P) {
   P.addSimplifyCFG();
   // Optimize access markers for better LICM: might merge accesses
   // It will also set the no_nested_conflict for dynamic accesses
+  P.addAccessEnforcementReleaseSinking();
   P.addAccessEnforcementOpts();
   P.addHighLevelLICM();
   // Simplify CFG after LICM that creates new exit blocks
@@ -202,6 +203,7 @@ void addHighLevelLoopOptPasses(SILPassPipelinePlan &P) {
   // LICM might have added new merging potential by hoisting
   // we don't want to restart the pipeline - ignore the
   // potential of merging out of two loops
+  P.addAccessEnforcementReleaseSinking();
   P.addAccessEnforcementOpts();
   // Start of loop unrolling passes.
   P.addArrayCountPropagation();
@@ -465,6 +467,7 @@ static void addLateLoopOptPassPipeline(SILPassPipelinePlan &P) {
   P.addCodeSinking();
   // Optimize access markers for better LICM: might merge accesses
   // It will also set the no_nested_conflict for dynamic accesses
+  P.addAccessEnforcementReleaseSinking();
   P.addAccessEnforcementOpts();
   P.addLICM();
   // Simplify CFG after LICM that creates new exit blocks
@@ -472,6 +475,7 @@ static void addLateLoopOptPassPipeline(SILPassPipelinePlan &P) {
   // LICM might have added new merging potential by hoisting
   // we don't want to restart the pipeline - ignore the
   // potential of merging out of two loops
+  P.addAccessEnforcementReleaseSinking();
   P.addAccessEnforcementOpts();
 
   // Optimize overflow checks.
@@ -494,6 +498,7 @@ static void addLateLoopOptPassPipeline(SILPassPipelinePlan &P) {
 // - don't require IRGen information.
 static void addLastChanceOptPassPipeline(SILPassPipelinePlan &P) {
   // Optimize access markers for improved IRGen after all other optimizations.
+  P.addAccessEnforcementReleaseSinking();
   P.addAccessEnforcementOpts();
   P.addAccessEnforcementWMO();
   P.addAccessEnforcementDom();

lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp

@@ -0,0 +1,124 @@
+//===--- AccessEnforcementReleaseSinking.cpp - release sinking opt ---===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+///
+/// This function pass sinks releases out of access scopes.
+///
+/// General case:
+/// begin_access A
+/// ...
+/// strong_release / release_value / destroy
+/// end_access
+///
+/// The release instruction can be sunk below the end_access instruction,
+/// This extends the lifetime of the released value, but, might allow us to
+/// Mark the access scope as no nested conflict.
+//===----------------------------------------------------------------------===//
+
+#define DEBUG_TYPE "access-enforcement-release"
+
+#include "swift/SIL/ApplySite.h"
+#include "swift/SIL/DebugUtils.h"
+#include "swift/SIL/SILFunction.h"
+#include "swift/SILOptimizer/PassManager/Transforms.h"
+
+using namespace swift;
+
+// Returns a bool: If this is a "sinkable" instruction type for this opt
+static bool isSinkable(SILInstruction &inst) {
+  switch (inst.getKind()) {
+  default:
+    return false;
+  case SILInstructionKind::DestroyValueInst:
+  case SILInstructionKind::ReleaseValueInst:
+  case SILInstructionKind::ReleaseValueAddrInst:
+  case SILInstructionKind::StrongReleaseInst:
+  case SILInstructionKind::UnmanagedReleaseValueInst:
+    return true;
+  }
+}
+
+// Returns a bool: If this is a "barrier" instruction for this opt
+static bool isBarrier(SILInstruction &inst) {
+  switch (inst.getKind()) {
+  default:
+    return FullApplySite::isa(&inst) != FullApplySite();
+  case SILInstructionKind::BeginAccessInst:
+  case SILInstructionKind::BeginUnpairedAccessInst:
+  case SILInstructionKind::IsUniqueInst:
+    return true;
+  }
+}
+
+// Processes a block bottom-up, keeping a lookout for end_access instructions
+// If we encounter a "barrier" we clear out the current end_access
+// If we encounter a "release", and we have a current end_access, we sink it
+static void processBlock(SILBasicBlock &block) {
+  EndAccessInst *bottomEndAccessInst = nullptr;
+  for (auto reverseIt = block.rbegin(); reverseIt != block.rend();
+       ++reverseIt) {
+    SILInstruction &currIns = *reverseIt;
+    if (auto *currEAI = dyn_cast<EndAccessInst>(&currIns)) {
+      if (!bottomEndAccessInst) {
+        bottomEndAccessInst = currEAI;
+      }
+    } else if (isBarrier(currIns)) {
+      LLVM_DEBUG(llvm::dbgs() << "Found a barrier " << currIns
+                              << ", clearing last seen end_access\n");
+      bottomEndAccessInst = nullptr;
+    } else if (isSinkable(currIns)) {
+      LLVM_DEBUG(llvm::dbgs()
+                 << "Found a sinkable instruction " << currIns << "\n");
+      if (!bottomEndAccessInst) {
+        LLVM_DEBUG(
+            llvm::dbgs()
+            << "Cannot be sunk: no open barrier-less end_access found\n");
+        continue;
+      }
+      LLVM_DEBUG(llvm::dbgs() << "Moving sinkable instruction below "
+                              << *bottomEndAccessInst << "\n");
+      // We need to avoid iterator invalidation:
+      // We know this is not the last instruction of the block:
+      // 1) not a TermInst
+      // 2) bottomEndAccessInst != nil
+      assert(reverseIt != block.rbegin() &&
+             "Did not expect a sinkable instruction at block's end");
+      // Go back to previous iteration
+      auto prevIt = reverseIt;
+      --prevIt;
+      // Move the instruction after the end_access
+      currIns.moveAfter(bottomEndAccessInst);
+      // make reverseIt into a valid iterator again
+      reverseIt = prevIt;
+    }
+  }
+}
+
+namespace {
+struct AccessEnforcementReleaseSinking : public SILFunctionTransform {
+  void run() override {
+    SILFunction *F = getFunction();
+    if (F->empty())
+      return;
+
+    LLVM_DEBUG(llvm::dbgs() << "Running AccessEnforcementReleaseSinking on "
+                            << F->getName() << "\n");
+
+    for (SILBasicBlock &currBB : *F) {
+      processBlock(currBB);
+    }
+  }
+};
+} // namespace
+
+SILTransform *swift::createAccessEnforcementReleaseSinking() {
+  return new AccessEnforcementReleaseSinking();
+}

lib/SILOptimizer/Transforms/CMakeLists.txt

@@ -2,6 +2,7 @@ silopt_register_sources(
   ARCCodeMotion.cpp
   AccessEnforcementDom.cpp
   AccessEnforcementOpts.cpp
+  AccessEnforcementReleaseSinking.cpp 
   AccessEnforcementWMO.cpp
   AllocBoxToStack.cpp
   ArrayCountPropagation.cpp

test/SILOptimizer/access_sink.sil

@@ -0,0 +1,217 @@
+// RUN: %target-sil-opt -access-enforcement-release -assume-parsing-unqualified-ownership-sil %s -enable-sil-verify-all | %FileCheck %s
+//
+// Test the AccessEnforcementReleaseSinking pass in isolation.
+// This ensures that no upstream passes have removed SIL-level access markers
+// that are required to ensure the pass is not overly optimistic.
+
+sil_stage canonical
+
+import Builtin
+import Swift
+import SwiftShims
+
+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 testSimpleRelease() {
+// Checks the simple case of release sinking
+//
+// CHECK-LABEL: sil @testSimpleRelease : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-LABEL: } // end sil function 'testSimpleRelease'
+sil @testSimpleRelease : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testMultiBlocklSimpleRelease() {
+// Checks the simple case of release sinking with the begin_access in a different block
+//
+// CHECK-LABEL: sil @testMultiBlocklSimpleRelease : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: br bb1
+// CHECK: bb1
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-LABEL: } // end sil function 'testMultiBlocklSimpleRelease'
+sil @testMultiBlocklSimpleRelease : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  br bb1
+  
+bb1:
+  release_value %2 : $X
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testMultiBlocklBailOnRelease() {
+// Checks bailing (for now) on the simple case due to the release being in a different block
+//
+// CHECK-LABEL: sil @testMultiBlocklBailOnRelease : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-NEXT: br bb1
+// CHECK: bb1
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-LABEL: } // end sil function 'testMultiBlocklBailOnRelease'
+sil @testMultiBlocklBailOnRelease : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  br bb1
+  
+bb1:
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testApplyBarrier() {
+// Checks we don't sink across apply-site barrier
+//
+// CHECK-LABEL: sil @testApplyBarrier : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK: apply
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-LABEL: } // end sil function 'testApplyBarrier'
+sil @testApplyBarrier : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  %u0 = function_ref @globalAddressor : $@convention(thin) () -> Builtin.RawPointer
+  %u1 = apply %u0() : $@convention(thin) () -> Builtin.RawPointer
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testUniquenessBarrier() {
+// Checks we don't sink across a uniqueness check
+//
+// CHECK-LABEL: sil @testUniquenessBarrier : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-NEXT: is_unique
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-LABEL: } // end sil function 'testUniquenessBarrier'
+sil @testUniquenessBarrier : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  is_unique %1 : $*X
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testBeginBarrier() {
+// Checks we don't sink across begin_access barrier
+//
+// CHECK-LABEL: sil @testBeginBarrier : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-NEXT: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: end_access [[BEGIN2]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-LABEL: } // end sil function 'testBeginBarrier'
+sil @testBeginBarrier : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  %b0 = begin_access [modify] [dynamic] %0 : $*X
+  end_access %b0 : $*X
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testSinkCrossMultiEnds() {
+// Checks that we choose the *bottom* end_access when sinking
+//
+// CHECK-LABEL: sil @testSinkCrossMultiEnds : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: end_access [[BEGIN2]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-LABEL: } // end sil function 'testSinkCrossMultiEnds'
+sil @testSinkCrossMultiEnds : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %b0 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  end_access %b0 : $*X
+  end_access %1 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}
+
+// public func testSinkAfterBarrierEncounter() {
+// Checks that we sink after barrier resetting
+//
+// CHECK-LABEL: sil @testSinkAfterBarrierEncounter : $@convention(thin) () -> () {
+// CHECK: [[GLOBAL:%.*]] = global_addr @globalX : $*X
+// CHECK-NEXT: [[BEGIN:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-NEXT: [[LOADED:%.*]] = load [[BEGIN]] : $*X
+// CHECK-NEXT: end_access [[BEGIN]] : $*X
+// CHECK-NEXT: release_value [[LOADED]]
+// CHECK-NEXT: [[BEGIN2:%.*]] = begin_access [modify] [dynamic] [[GLOBAL]] : $*X
+// CHECK-LABEL: } // end sil function 'testSinkAfterBarrierEncounter'
+sil @testSinkAfterBarrierEncounter : $@convention(thin) () -> () {
+bb0:
+  %0 = global_addr @globalX: $*X
+  %1 = begin_access [modify] [dynamic] %0 : $*X
+  %2 = load %1 : $*X
+  release_value %2 : $X
+  end_access %1 : $*X
+  %b0 = begin_access [modify] [dynamic] %0 : $*X
+  end_access %b0 : $*X
+  %ret = tuple ()
+  return %ret : $()
+}