From 32da4f9f4518ff58b46552bd30e9e9a31443de1c Mon Sep 17 00:00:00 2001 From: Joe Shajrawi Date: Wed, 31 Oct 2018 12:21:57 -0700 Subject: [PATCH] [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. --- .../swift/SILOptimizer/PassManager/Passes.def | 2 + lib/SILOptimizer/PassManager/PassPipeline.cpp | 5 + .../AccessEnforcementReleaseSinking.cpp | 124 ++++++++++ lib/SILOptimizer/Transforms/CMakeLists.txt | 1 + test/SILOptimizer/access_sink.sil | 217 ++++++++++++++++++ 5 files changed, 349 insertions(+) create mode 100644 lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp create mode 100644 test/SILOptimizer/access_sink.sil diff --git a/include/swift/SILOptimizer/PassManager/Passes.def b/include/swift/SILOptimizer/PassManager/Passes.def index 2258864f42a..f14f51d92f2 100644 --- a/include/swift/SILOptimizer/PassManager/Passes.def +++ b/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", diff --git a/lib/SILOptimizer/PassManager/PassPipeline.cpp b/lib/SILOptimizer/PassManager/PassPipeline.cpp index e46f54d41b0..384bd4c11ea 100644 --- a/lib/SILOptimizer/PassManager/PassPipeline.cpp +++ b/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(); diff --git a/lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp b/lib/SILOptimizer/Transforms/AccessEnforcementReleaseSinking.cpp new file mode 100644 index 00000000000..e0d3405ef39 --- /dev/null +++ b/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(&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(); +} diff --git a/lib/SILOptimizer/Transforms/CMakeLists.txt b/lib/SILOptimizer/Transforms/CMakeLists.txt index e3bf6945aab..4ded5f556a3 100644 --- a/lib/SILOptimizer/Transforms/CMakeLists.txt +++ b/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 diff --git a/test/SILOptimizer/access_sink.sil b/test/SILOptimizer/access_sink.sil new file mode 100644 index 00000000000..4d386677cd8 --- /dev/null +++ b/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 : $() +}