From bdc1f404e6c243705335aa93d7433e59a349eb9d Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Thu, 19 May 2022 08:32:05 -0700 Subject: [PATCH 1/3] [Gardening] Recapitalized type. --- lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp index a37d7c36d933f..602f85b7e01a0 100644 --- a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp +++ b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp @@ -139,17 +139,17 @@ struct DeinitBarriers final { /// place they can be hoisted to. /// /// Implements BackwardReachability::BlockReachability. -class DataFlow final { +class Dataflow final { Context const &context; Usage const &uses; DeinitBarriers &result; enum class Classification { Barrier, Copy, Other }; - BackwardReachability reachability; + BackwardReachability reachability; public: - DataFlow(Context const &context, Usage const &uses, DeinitBarriers &result) + Dataflow(Context const &context, Usage const &uses, DeinitBarriers &result) : context(context), uses(uses), result(result), reachability(&context.function, *this) { // Seed reachability with the scope ending uses from which the backwards @@ -158,13 +158,13 @@ class DataFlow final { reachability.initLastUse(end); } } - DataFlow(DataFlow const &) = delete; - DataFlow &operator=(DataFlow const &) = delete; + Dataflow(Dataflow const &) = delete; + Dataflow &operator=(Dataflow const &) = delete; void run() { reachability.solveBackward(); } private: - friend class BackwardReachability; + friend class BackwardReachability; bool hasReachableBegin(SILBasicBlock *block) { return result.hoistingReachesBeginBlocks.contains(block); @@ -207,8 +207,8 @@ bool isSimpleExtendedIntroducerDef(Context const &context, SILValue value) { } } -DataFlow::Classification -DataFlow::classifyInstruction(SILInstruction *instruction) { +Dataflow::Classification +Dataflow::classifyInstruction(SILInstruction *instruction) { if (instruction == &context.introducer) { return Classification::Barrier; } @@ -226,7 +226,7 @@ DataFlow::classifyInstruction(SILInstruction *instruction) { return Classification::Other; } -bool DataFlow::classificationIsBarrier(Classification classification) { +bool Dataflow::classificationIsBarrier(Classification classification) { switch (classification) { case Classification::Barrier: return true; @@ -237,7 +237,7 @@ bool DataFlow::classificationIsBarrier(Classification classification) { llvm_unreachable("exhaustive switch not exhaustive?!"); } -void DataFlow::visitedInstruction(SILInstruction *instruction, +void Dataflow::visitedInstruction(SILInstruction *instruction, Classification classification) { assert(classifyInstruction(instruction) == classification); switch (classification) { @@ -253,13 +253,13 @@ void DataFlow::visitedInstruction(SILInstruction *instruction, llvm_unreachable("exhaustive switch not exhaustive?!"); } -bool DataFlow::checkReachableBarrier(SILInstruction *instruction) { +bool Dataflow::checkReachableBarrier(SILInstruction *instruction) { auto classification = classifyInstruction(instruction); visitedInstruction(instruction, classification); return classificationIsBarrier(classification); } -bool DataFlow::checkReachablePhiBarrier(SILBasicBlock *block) { +bool Dataflow::checkReachablePhiBarrier(SILBasicBlock *block) { assert(llvm::all_of(block->getArguments(), [&](auto argument) { return PhiValue(argument); })); @@ -397,7 +397,7 @@ bool run(Context &context) { return false; DeinitBarriers barriers(context); - DataFlow flow(context, usage, barriers); + Dataflow flow(context, usage, barriers); flow.run(); Rewriter rewriter(context, usage, barriers); From 180095ae3a1b8db200d36a79ba1126768e19bce6 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 16 May 2022 12:57:34 -0700 Subject: [PATCH 2/3] [ShrinkBorrowScope] Adopt iterative dataflow. Adopt IterativeBackwardReachability. Enables hoisting end_borrow instructions over loops. That in turn enables hoisting destroy_values over those same loops. rdar://93186505 --- lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp | 152 +++++++++---------- test/SILOptimizer/shrink_borrow_scope.sil | 6 +- 2 files changed, 73 insertions(+), 85 deletions(-) diff --git a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp index 602f85b7e01a0..3fb261bd2d813 100644 --- a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp +++ b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp @@ -49,6 +49,8 @@ struct Context final { /// introducer->getOperand() SILValue const borrowee; + SILBasicBlock *defBlock; + SILFunction &function; /// The copy_value instructions that the utility creates or changes. @@ -62,7 +64,8 @@ struct Context final { SmallVectorImpl &modifiedCopyValueInsts, InstructionDeleter &deleter) : introducer(introducer), borrowedValue(BorrowedValue(&introducer)), - borrowee(introducer.getOperand()), function(*introducer.getFunction()), + borrowee(introducer.getOperand()), defBlock(introducer.getParent()), + function(*introducer.getFunction()), modifiedCopyValueInsts(modifiedCopyValueInsts), deleter(deleter) {} Context(Context const &) = delete; Context &operator=(Context const &) = delete; @@ -75,7 +78,7 @@ struct Usage final { SmallPtrSet users; // The instructions from which the shrinking starts, the scope ending // instructions. - llvm::SmallSetVector ends; + llvm::SmallVector ends; Usage(){}; Usage(Usage const &) = delete; @@ -95,7 +98,7 @@ bool findUsage(Context const &context, Usage &usage) { // If a scope ending instruction is not an end_borrow, bail out. if (!isa(instruction)) return false; - usage.ends.insert(instruction); + usage.ends.push_back(instruction); } SmallVector uses; @@ -112,25 +115,21 @@ bool findUsage(Context const &context, Usage &usage) { /// How end_borrow hoisting is obstructed. struct DeinitBarriers final { - /// Blocks up to "before the beginning" of which hoisting was able to proceed. - BasicBlockSetVector hoistingReachesBeginBlocks; - - /// Blocks to "after the end" of which hoisting was able to proceed. - BasicBlockSet hoistingReachesEndBlocks; - /// Copies to be rewritten as copies of %borrowee. SmallVector copies; /// Instructions above which end_borrows cannot be hoisted. - SmallVector barriers; + SmallVector instructions; /// Blocks one of whose phis is a barrier and consequently out of which /// end_borrows cannot be hoisted. - SmallVector phiBarriers; + SmallVector phis; + + /// Blocks whose single predecessors has another successor to the top of which + /// end_borrows cannot be hoisted. + SmallVector blocks; - DeinitBarriers(Context &context) - : hoistingReachesBeginBlocks(&context.function), - hoistingReachesEndBlocks(&context.function) {} + DeinitBarriers(Context &context) {} DeinitBarriers(DeinitBarriers const &) = delete; DeinitBarriers &operator=(DeinitBarriers const &) = delete; }; @@ -138,55 +137,60 @@ struct DeinitBarriers final { /// Works backwards from the current location of end_borrows to the earliest /// place they can be hoisted to. /// -/// Implements BackwardReachability::BlockReachability. +/// Implements IterativeBackwardReachability::Effects. +/// Implements IterativeBackwardReachability::findBarrier::Visitor. class Dataflow final { +public: + using Reachability = IterativeBackwardReachability; + using Effect = Reachability::Effect; + +private: Context const &context; Usage const &uses; - DeinitBarriers &result; + DeinitBarriers &barriers; + Reachability::Result result; + Reachability reachability; + SmallPtrSet barrierAccessScopes; + bool recordCopies = false; enum class Classification { Barrier, Copy, Other }; - BackwardReachability reachability; - public: - Dataflow(Context const &context, Usage const &uses, DeinitBarriers &result) - : context(context), uses(uses), result(result), - reachability(&context.function, *this) { - // Seed reachability with the scope ending uses from which the backwards - // data flow will begin. - for (auto *end : uses.ends) { - reachability.initLastUse(end); - } - } + Dataflow(Context const &context, Usage const &uses, DeinitBarriers &barriers) + : context(context), uses(uses), barriers(barriers), + result(&context.function), + reachability(&context.function, context.defBlock, *this, result) {} Dataflow(Dataflow const &) = delete; Dataflow &operator=(Dataflow const &) = delete; - void run() { reachability.solveBackward(); } + void run(); private: - friend class BackwardReachability; + friend Reachability; - bool hasReachableBegin(SILBasicBlock *block) { - return result.hoistingReachesBeginBlocks.contains(block); - } + Classification classifyInstruction(SILInstruction *); - void markReachableBegin(SILBasicBlock *block) { - result.hoistingReachesBeginBlocks.insert(block); - } + bool classificationIsBarrier(Classification); - void markReachableEnd(SILBasicBlock *block) { - result.hoistingReachesEndBlocks.insert(block); - } + /// Implements IterativeBackwardReachability::Effects. - Classification classifyInstruction(SILInstruction *); + ArrayRef gens() { return uses.ends; } - bool classificationIsBarrier(Classification); + Effect effectForInstruction(SILInstruction *); - void visitedInstruction(SILInstruction *, Classification); + Effect effectForPhi(SILBasicBlock *); - bool checkReachableBarrier(SILInstruction *); + /// IterativeBackwardReachability::findBarrier::Visitor. - bool checkReachablePhiBarrier(SILBasicBlock *); + void visitBarrierInstruction(SILInstruction *instruction) { + barriers.instructions.push_back(instruction); + } + + void visitBarrierPhi(SILBasicBlock *block) { barriers.phis.push_back(block); } + + void visitBarrierBlock(SILBasicBlock *block) { + barriers.blocks.push_back(block); + } }; /// Whether the specified value is %lifetime or its iterated copy_value. @@ -237,29 +241,17 @@ bool Dataflow::classificationIsBarrier(Classification classification) { llvm_unreachable("exhaustive switch not exhaustive?!"); } -void Dataflow::visitedInstruction(SILInstruction *instruction, - Classification classification) { - assert(classifyInstruction(instruction) == classification); - switch (classification) { - case Classification::Barrier: - result.barriers.push_back(instruction); - return; - case Classification::Copy: - result.copies.push_back(cast(instruction)); - return; - case Classification::Other: - return; - } - llvm_unreachable("exhaustive switch not exhaustive?!"); -} - -bool Dataflow::checkReachableBarrier(SILInstruction *instruction) { +Dataflow::Effect Dataflow::effectForInstruction(SILInstruction *instruction) { + if (llvm::find(uses.ends, instruction) != uses.ends.end()) + return Effect::Gen(); auto classification = classifyInstruction(instruction); - visitedInstruction(instruction, classification); - return classificationIsBarrier(classification); + if (recordCopies && classification == Classification::Copy) + barriers.copies.push_back(cast(instruction)); + return classificationIsBarrier(classification) ? Effect::Kill() + : Effect::NoEffect(); } -bool Dataflow::checkReachablePhiBarrier(SILBasicBlock *block) { +Dataflow::Effect Dataflow::effectForPhi(SILBasicBlock *block) { assert(llvm::all_of(block->getArguments(), [&](auto argument) { return PhiValue(argument); })); @@ -268,10 +260,14 @@ bool Dataflow::checkReachablePhiBarrier(SILBasicBlock *block) { return classificationIsBarrier( classifyInstruction(predecessor->getTerminator())); }); - if (isBarrier) { - result.phiBarriers.push_back(block); - } - return isBarrier; + return isBarrier ? Effect::Kill() : Effect::NoEffect(); +} + +void Dataflow::run() { + reachability.initialize(); + reachability.solve(); + recordCopies = true; + reachability.findBarriers(*this); } /// Hoist the scope ends of %lifetime, rewriting copies and borrows along the @@ -311,7 +307,7 @@ bool Rewriter::run() { // A block is a phi barrier iff any of its predecessors' terminators get // classified as barriers. That happens when a copy of %lifetime is passed // to a phi. - for (auto *block : barriers.phiBarriers) { + for (auto *block : barriers.phis) { madeChange |= createEndBorrow(&block->front()); } @@ -324,15 +320,11 @@ bool Rewriter::run() { // of a block P's successors B had reachable beginnings. If any of them // didn't, then BackwardReachability::meetOverSuccessors would never have // returned true for P, so none of its instructions would ever have been - // classified (except for via checkReachablePhiBarrier, which doesn't record - // terminator barriers). - for (auto instruction : barriers.barriers) { + // classified (except for via effectForPhi, which doesn't record terminator + // barriers). + for (auto instruction : barriers.instructions) { if (auto *terminator = dyn_cast(instruction)) { auto successors = terminator->getParentBlock()->getSuccessorBlocks(); - // In order for the instruction to have been classified as a barrier, - // reachability would have had to reach the block containing it. - assert(barriers.hoistingReachesEndBlocks.contains( - terminator->getParentBlock())); for (auto *successor : successors) { madeChange |= createEndBorrow(&successor->front()); } @@ -356,12 +348,8 @@ bool Rewriter::run() { // P not having a reachable end--see BackwardReachability::meetOverSuccessors. // // control-flow-boundary(B) := beginning-reachable(B) && !end-reachable(P) - for (auto *block : barriers.hoistingReachesBeginBlocks) { - if (auto *predecessor = block->getSinglePredecessorBlock()) { - if (!barriers.hoistingReachesEndBlocks.contains(predecessor)) { - madeChange |= createEndBorrow(&block->front()); - } - } + for (auto *block : barriers.blocks) { + madeChange |= createEndBorrow(&block->front()); } if (madeChange) { @@ -379,7 +367,7 @@ bool Rewriter::run() { bool Rewriter::createEndBorrow(SILInstruction *insertionPoint) { if (auto *ebi = dyn_cast(insertionPoint)) { - if (uses.ends.contains(insertionPoint)) { + if (llvm::find(uses.ends, insertionPoint) != uses.ends.end()) { reusedEndBorrowInsts.insert(insertionPoint); return false; } diff --git a/test/SILOptimizer/shrink_borrow_scope.sil b/test/SILOptimizer/shrink_borrow_scope.sil index 42821d9535e60..553568e4b9be9 100644 --- a/test/SILOptimizer/shrink_borrow_scope.sil +++ b/test/SILOptimizer/shrink_borrow_scope.sil @@ -457,13 +457,14 @@ exit: // loop tests {{ // ============================================================================= -// Don't hoist over loop without uses. -// TODO: Eventually, we should hoist over such loops. +// Hoist over loop without uses. +// // CHECK-LABEL: sil [ossa] @hoist_over_loop_1 : {{.*}} { // CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $C): // CHECK: [[LIFETIME:%[^,]+]] = begin_borrow [[INSTANCE]] // CHECK: [[CALLEE_GUARANTEED:%[^,]+]] = function_ref @callee_guaranteed // CHECK: {{%[^,]+}} = apply [[CALLEE_GUARANTEED]]([[LIFETIME]]) +// CHECK: end_borrow [[LIFETIME]] // CHECK: br [[LOOP_HEADER:bb[0-9]+]] // CHECK: [[LOOP_HEADER]]: // CHECK: br [[LOOP_BODY:bb[0-9]+]] @@ -474,7 +475,6 @@ exit: // CHECK: [[LOOP_BACKEDGE]]: // CHECK: br [[LOOP_HEADER]] // CHECK: [[EXIT]]: -// CHECK: end_borrow [[LIFETIME]] // CHECK: return [[INSTANCE]] // CHECK-LABEL: } // end sil function 'hoist_over_loop_1' sil [ossa] @hoist_over_loop_1 : $@convention(thin) (@owned C) -> @owned C { From 3c7ad6013a64b4f998f25a2a5f35c40e11d9fabb Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Fri, 20 May 2022 09:20:28 -0700 Subject: [PATCH 3/3] [ShrinkBorrowScope] Adopt VisitBarrierAccessScopes. Avoids hoisting borrow scopes into unrelated access scopes which could introduce exclusivity violations. rdar://93060369 --- lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp | 57 ++++++++++++++- test/SILOptimizer/shrink_borrow_scope.sil | 73 ++++++++++++++++++++ 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp index 3fb261bd2d813..064eb410dab55 100644 --- a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp +++ b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp @@ -21,6 +21,7 @@ #include "swift/SIL/SILBasicBlock.h" #include "swift/SIL/SILInstruction.h" #include "swift/SILOptimizer/Analysis/Reachability.h" +#include "swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h" #include "swift/SILOptimizer/Utils/CanonicalizeBorrowScope.h" #include "swift/SILOptimizer/Utils/InstOptUtils.h" #include "swift/SILOptimizer/Utils/InstructionDeleter.h" @@ -134,11 +135,14 @@ struct DeinitBarriers final { DeinitBarriers &operator=(DeinitBarriers const &) = delete; }; +class BarrierAccessScopeFinder; + /// Works backwards from the current location of end_borrows to the earliest /// place they can be hoisted to. /// /// Implements IterativeBackwardReachability::Effects. /// Implements IterativeBackwardReachability::findBarrier::Visitor. +/// Implements VisitBarrierAccessScopes::Effects class Dataflow final { public: using Reachability = IterativeBackwardReachability; @@ -167,12 +171,15 @@ class Dataflow final { private: friend Reachability; + friend class BarrierAccessScopeFinder; + friend class VisitBarrierAccessScopes; Classification classifyInstruction(SILInstruction *); bool classificationIsBarrier(Classification); - /// Implements IterativeBackwardReachability::Effects. + /// IterativeBackwardReachability::Effects + /// VisitBarrierAccessScopes::Effects ArrayRef gens() { return uses.ends; } @@ -180,6 +187,14 @@ class Dataflow final { Effect effectForPhi(SILBasicBlock *); + /// VisitBarrierAccessScopes::Effects + + auto localGens() { return result.localGens; } + + bool isLocalGen(SILInstruction *instruction) { + return result.localGens.contains(instruction); + } + /// IterativeBackwardReachability::findBarrier::Visitor. void visitBarrierInstruction(SILInstruction *instruction) { @@ -224,6 +239,11 @@ Dataflow::classifyInstruction(SILInstruction *instruction) { if (uses.users.contains(instruction)) { return Classification::Barrier; } + if (auto *eai = dyn_cast(instruction)) { + return barrierAccessScopes.contains(eai->getBeginAccess()) + ? Classification::Barrier + : Classification::Other; + } if (isDeinitBarrier(instruction)) { return Classification::Barrier; } @@ -263,8 +283,43 @@ Dataflow::Effect Dataflow::effectForPhi(SILBasicBlock *block) { return isBarrier ? Effect::Kill() : Effect::NoEffect(); } +/// Finds end_access instructions which are barriers to hoisting because the +/// access scopes they contain barriers to hoisting. Hoisting end_borrows into +/// such access scopes could introduce exclusivity violations. +/// +/// Implements BarrierAccessScopeFinder::Visitor +class BarrierAccessScopeFinder final { + using Impl = VisitBarrierAccessScopes; + Context const &context; + Impl impl; + Dataflow &dataflow; + +public: + BarrierAccessScopeFinder(Context const &context, Dataflow &dataflow) + : context(context), impl(&context.function, dataflow, *this), + dataflow(dataflow) {} + + void find() { impl.visit(); } + +private: + friend Impl; + + bool isInRegion(SILBasicBlock *block) { + return dataflow.result.discoveredBlocks.contains(block); + } + + void visitBarrierAccessScope(BeginAccessInst *bai) { + dataflow.barrierAccessScopes.insert(bai); + for (auto *eai : bai->getEndAccesses()) { + dataflow.reachability.addKill(eai); + } + } +}; + void Dataflow::run() { reachability.initialize(); + BarrierAccessScopeFinder finder(context, *this); + finder.find(); reachability.solve(); recordCopies = true; reachability.findBarriers(*this); diff --git a/test/SILOptimizer/shrink_borrow_scope.sil b/test/SILOptimizer/shrink_borrow_scope.sil index 553568e4b9be9..59e0f28b9986d 100644 --- a/test/SILOptimizer/shrink_borrow_scope.sil +++ b/test/SILOptimizer/shrink_borrow_scope.sil @@ -1097,3 +1097,76 @@ bb0(%0 : $*ClassWrapper): // ============================================================================= // instruction tests }} // ============================================================================= + +// ============================================================================= +// access scope tests {{ +// ============================================================================= + +// Don't hoist into an access scope that contains a barrier. +// +// CHECK-LABEL: sil [ossa] @nofold_scoped_load_barrier : {{.*}} { +// CHECK: end_access +// CHECK: end_access +// CHECK: end_borrow +// CHECK-LABEL: // end sil function 'nofold_scoped_load_barrier' +sil [ossa] @nofold_scoped_load_barrier : $@convention(thin) (@owned C, @owned C) -> (@owned C) { +entry(%instance : @owned $C, %other : @owned $C): + %lifetime = begin_borrow [lexical] %instance : $C + apply undef(%lifetime) : $@convention(thin) (@guaranteed C) -> () + %addr = alloc_stack $C + %store_scope = begin_access [modify] [static] %addr : $*C + store %other to [init] %store_scope : $*C + end_access %store_scope : $*C + %load_scope = begin_access [read] [static] %addr : $*C + %value = load [copy] %load_scope : $*C + %barrier = function_ref @barrier : $@convention(thin) () -> () + apply %barrier() : $@convention(thin) () -> () + end_access %load_scope : $*C + destroy_addr %addr : $*C + dealloc_stack %addr : $*C + end_borrow %lifetime : $C + destroy_value %instance : $C + return %value : $C +} + +// Access scopes that are open at barrier blocks are barriers. Otherwise, we +// would hoist end_borrows into the scopes when the end_borrows are hoisted up +// to the begin of blocks whose predecessor is the barrier block. +// +// CHECK-LABEL: sil [ossa] @nohoist_into_access_scope_barred_by_barrier_block : {{.*}} { +// CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $C, [[INOUT:%[^,]+]] : $*C): +// CHECK: [[LIFETIME:%[^,]+]] = begin_borrow [lexical] [[INSTANCE]] +// CHECK: [[SCOPE:%[^,]+]] = begin_access [modify] [static] [[INOUT]] : $*C +// CHECK: cond_br undef, [[LEFT:bb[0-9]+]], +// CHECK: [[LEFT]]: +// CHECK: end_access [[SCOPE]] : $*C +// CHECK-NEXT: end_borrow [[LIFETIME]] : $C +// CHECK-LABEL: } // end sil function 'nohoist_into_access_scope_barred_by_barrier_block' +sil [ossa] @nohoist_into_access_scope_barred_by_barrier_block : $@convention(thin) (@owned C, @inout C) -> () { +entry(%instance : @owned $C, %second : $*C): + %lifetime = begin_borrow [lexical] %instance : $C + %scope = begin_access [modify] [static] %second : $*C + cond_br undef, left, right + +left: + end_access %scope : $*C + %ignore = tuple () + end_borrow %lifetime : $C + destroy_value %instance : $C + br exit + +right: + end_access %scope : $*C + apply undef(%lifetime) : $@convention(thin) (@guaranteed C) -> () + end_borrow %lifetime : $C + destroy_value %instance : $C + br exit + +exit: + %retval = tuple () + return %retval : $() +} + +// ============================================================================= +// access scope tests }} +// =============================================================================