diff --git a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp index 2467b54ccb268..254008483c0a5 100644 --- a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp +++ b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp @@ -20,6 +20,7 @@ #include "swift/SIL/SILInstruction.h" #include "swift/SIL/SILValue.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" @@ -43,6 +44,8 @@ struct Context final { /// value->getDefiningInstruction() SILInstruction *const definition; + SILBasicBlock *defBlock; + SILFunction &function; InstructionDeleter &deleter; @@ -50,7 +53,8 @@ struct Context final { Context(SILValue const &value, SILFunction &function, InstructionDeleter &deleter) : value(value), definition(value->getDefiningInstruction()), - function(function), deleter(deleter) { + defBlock(value->getParentBlock()), function(function), + deleter(deleter) { assert(value->isLexical()); assert(value->getOwnershipKind() == OwnershipKind::Owned); } @@ -63,7 +67,7 @@ struct Usage final { /// Instructions which are users of the simple (i.e. not reborrowed) value. SmallPtrSet users; // The instructions from which the hoisting starts, the destroy_values. - llvm::SmallSetVector ends; + llvm::SmallVector ends; Usage(){}; Usage(Usage const &) = delete; @@ -85,7 +89,7 @@ bool findUsage(Context const &context, Usage &usage) { // flow and determine whether any were reused. They aren't uses over which // we can't hoist though. if (isa(use->getUser())) { - usage.ends.insert(use->getUser()); + usage.ends.push_back(use->getUser()); } else { usage.users.insert(use->getUser()); } @@ -95,95 +99,108 @@ bool findUsage(Context const &context, Usage &usage) { /// How destroy_value 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; - /// Instructions above which destroy_values cannot be hoisted. - SmallVector barriers; + SmallVector instructions; /// Blocks one of whose phis is a barrier and consequently out of which /// destroy_values cannot be hoisted. - SmallVector phiBarriers; + SmallVector phis; - DeinitBarriers(Context &context) - : hoistingReachesBeginBlocks(&context.function), - hoistingReachesEndBlocks(&context.function) {} + SmallVector blocks; + + DeinitBarriers(Context &context) {} DeinitBarriers(DeinitBarriers const &) = delete; DeinitBarriers &operator=(DeinitBarriers const &) = delete; }; +class BarrierAccessScopeFinder; + /// Works backwards from the current location of destroy_values to the earliest /// place they can be hoisted to. /// -/// Implements BackwardReachability::BlockReachability. -class DataFlow final { +/// Implements IterativeBackwardReachability::Effects +/// Implements IterativeBackwardReachability::bindBarriers::Visitor +/// Implements VisitBarrierAccessScopes::Effects +class Dataflow final { + using Reachability = IterativeBackwardReachability; + using Effect = Reachability::Effect; Context const &context; Usage const &uses; - DeinitBarriers &result; + DeinitBarriers &barriers; + Reachability::Result result; + Reachability reachability; + SmallPtrSet barrierAccessScopes; enum class Classification { Barrier, 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(DataFlow const &) = delete; - DataFlow &operator=(DataFlow const &) = delete; + 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; + friend class BarrierAccessScopeFinder; + friend class VisitBarrierAccessScopes; - 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); - } + /// IterativeBackwardReachability::Effects + /// VisitBarrierAccessScopes::Effects - Classification classifyInstruction(SILInstruction *); + ArrayRef gens() { return uses.ends; } - bool classificationIsBarrier(Classification); + Effect effectForInstruction(SILInstruction *); + Effect effectForPhi(SILBasicBlock *); + + /// VisitBarrierAccessScopes::Effects + + auto localGens() { return result.localGens; } - void visitedInstruction(SILInstruction *, Classification); + bool isLocalGen(SILInstruction *instruction) { + return result.localGens.contains(instruction); + } + + /// IterativeBackwardReachability::bindBarriers::Visitor + + void visitBarrierInstruction(SILInstruction *instruction) { + barriers.instructions.push_back(instruction); + } - bool checkReachableBarrier(SILInstruction *); + void visitBarrierPhi(SILBasicBlock *block) { barriers.phis.push_back(block); } - bool checkReachablePhiBarrier(SILBasicBlock *); + void visitBarrierBlock(SILBasicBlock *block) { + barriers.blocks.push_back(block); + } }; -DataFlow::Classification -DataFlow::classifyInstruction(SILInstruction *instruction) { +Dataflow::Classification +Dataflow::classifyInstruction(SILInstruction *instruction) { if (instruction == context.definition) { return Classification::Barrier; } 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; } return Classification::Other; } -bool DataFlow::classificationIsBarrier(Classification classification) { +bool Dataflow::classificationIsBarrier(Classification classification) { switch (classification) { case Classification::Barrier: return true; @@ -193,26 +210,15 @@ 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::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); + 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); })); @@ -221,10 +227,46 @@ bool DataFlow::checkReachablePhiBarrier(SILBasicBlock *block) { return classificationIsBarrier( classifyInstruction(predecessor->getTerminator())); }); - if (isBarrier) { - result.phiBarriers.push_back(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 destroy_values +/// into such access scopes could introduce exclusivity violations. +/// +/// Implements BarrierAccessScopeFinder::Visitor +class BarrierAccessScopeFinder final { + using Impl = VisitBarrierAccessScopes; + Impl impl; + Dataflow &dataflow; + +public: + BarrierAccessScopeFinder(Context const &context, Dataflow &dataflow) + : 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); + } } - return isBarrier; +}; + +void Dataflow::run() { + reachability.initialize(); + BarrierAccessScopeFinder finder(context, *this); + finder.find(); + reachability.solve(); + reachability.findBarriers(*this); } /// Hoist the destroy_values of %value. @@ -256,7 +298,7 @@ bool Rewriter::run() { // // A block is a phi barrier iff any of its predecessors' terminators get // classified as barriers. - for (auto *block : barriers.phiBarriers) { + for (auto *block : barriers.phis) { madeChange |= createDestroyValue(&block->front()); } @@ -271,13 +313,9 @@ bool Rewriter::run() { // 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) { + 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 |= createDestroyValue(&successor->front()); } @@ -301,12 +339,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 |= createDestroyValue(&block->front()); - } - } + for (auto *block : barriers.blocks) { + madeChange |= createDestroyValue(&block->front()); } if (madeChange) { @@ -324,7 +358,7 @@ bool Rewriter::run() { bool Rewriter::createDestroyValue(SILInstruction *insertionPoint) { if (auto *ebi = dyn_cast(insertionPoint)) { - if (uses.ends.contains(insertionPoint)) { + if (llvm::find(uses.ends, insertionPoint) != uses.ends.end()) { reusedDestroyValueInsts.insert(insertionPoint); return false; } @@ -342,7 +376,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); diff --git a/test/SILOptimizer/lexical_destroy_hoisting.sil b/test/SILOptimizer/lexical_destroy_hoisting.sil index 16e29693cc602..8cff942dcdc00 100644 --- a/test/SILOptimizer/lexical_destroy_hoisting.sil +++ b/test/SILOptimizer/lexical_destroy_hoisting.sil @@ -237,11 +237,11 @@ exit(%thing : @owned $C): // Don't hoist over loop without uses. // -// TODO: Eventually, we should hoist over such loops. // CHECK-LABEL: sil [ossa] @hoist_over_loop_1 : {{.*}} { // CHECK: {{bb[0-9]+}}([[INSTANCE:%[^,]+]] : @owned $C): // CHECK: [[CALLEE_GUARANTEED:%[^,]+]] = function_ref @callee_guaranteed // CHECK: apply [[CALLEE_GUARANTEED]]([[INSTANCE]]) +// CHECK: destroy_value [[INSTANCE]] // CHECK: br [[LOOP_HEADER:bb[0-9]+]] // CHECK: [[LOOP_HEADER]]: // CHECK: br [[LOOP_BODY:bb[0-9]+]] @@ -252,7 +252,6 @@ exit(%thing : @owned $C): // CHECK: [[LOOP_BACKEDGE]]: // CHECK: br [[LOOP_HEADER]] // CHECK: [[EXIT]]: -// CHECK: destroy_value [[INSTANCE]] // CHECK-LABEL: } // end sil function 'hoist_over_loop_1' sil [ossa] @hoist_over_loop_1 : $@convention(thin) (@owned C) -> () { entry(%instance: @owned $C): @@ -461,3 +460,67 @@ entry(%instance : @owned $C, %input : $S): // 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: destroy_value +// 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): + %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 + destroy_value %instance : $C + return %value : $C +} + +// Access scopes that are open at barrier blocks are barriers. Otherwise, we +// would hoist destroy_values into the scopes when the destroy_values 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: [[SCOPE:%[^,]+]] = begin_access [modify] [static] [[INOUT]] : $*C +// CHECK: cond_br undef, [[LEFT:bb[0-9]+]], +// CHECK: [[LEFT]]: +// CHECK: end_access [[SCOPE]] : $*C +// CHECK-NEXT: destroy_value [[INSTANCE]] : $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): + %scope = begin_access [modify] [static] %second : $*C + cond_br undef, left, right + +left: + end_access %scope : $*C + %ignore = tuple () + destroy_value %instance : $C + br exit + +right: + end_access %scope : $*C + apply undef(%instance) : $@convention(thin) (@owned C) -> () + br exit + +exit: + %retval = tuple () + return %retval : $() +} + +// ============================================================================= +// access scope tests }} +// =============================================================================