From 158db440fc3fe0bf32a4a062728dcd526827cbad Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Thu, 19 May 2022 08:41:54 -0700 Subject: [PATCH 1/3] [Gardening] Recapitalized type. --- .../Utils/LexicalDestroyHoisting.cpp | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp index 2467b54ccb268..f75498d8d09e6 100644 --- a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp +++ b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp @@ -119,17 +119,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, 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 @@ -138,13 +138,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); @@ -169,8 +169,8 @@ class DataFlow final { bool checkReachablePhiBarrier(SILBasicBlock *); }; -DataFlow::Classification -DataFlow::classifyInstruction(SILInstruction *instruction) { +Dataflow::Classification +Dataflow::classifyInstruction(SILInstruction *instruction) { if (instruction == context.definition) { return Classification::Barrier; } @@ -183,7 +183,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; @@ -193,7 +193,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) { @@ -206,13 +206,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); })); @@ -342,7 +342,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 fdc35a113872602ca405f035eea81dcbc2a2ab97 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 16 May 2022 14:51:23 -0700 Subject: [PATCH 2/3] [LexicalDestroyHoisting] Adopt iterative dataflow. Adopt IterativeBackwardReachability. Enables hoisting destroy_values over loops. rdar://93369506 --- .../Utils/LexicalDestroyHoisting.cpp | 134 ++++++++---------- .../SILOptimizer/lexical_destroy_hoisting.sil | 3 +- 2 files changed, 58 insertions(+), 79 deletions(-) diff --git a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp index f75498d8d09e6..36a197220d5ea 100644 --- a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp +++ b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp @@ -43,6 +43,8 @@ struct Context final { /// value->getDefiningInstruction() SILInstruction *const definition; + SILBasicBlock *defBlock; + SILFunction &function; InstructionDeleter &deleter; @@ -50,7 +52,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 +66,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 +88,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,22 +98,16 @@ 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; + + SmallVector blocks; - DeinitBarriers(Context &context) - : hoistingReachesBeginBlocks(&context.function), - hoistingReachesEndBlocks(&context.function) {} + DeinitBarriers(Context &context) {} DeinitBarriers(DeinitBarriers const &) = delete; DeinitBarriers &operator=(DeinitBarriers const &) = delete; }; @@ -118,55 +115,54 @@ struct DeinitBarriers final { /// Works backwards from the current location of destroy_values to the earliest /// place they can be hoisted to. /// -/// Implements BackwardReachability::BlockReachability. +/// Implements IterativeBackwardReachability::Effects +/// Implements IterativeBackwardReachability::bindBarriers::Visitor 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; 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(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); - } + /// IterativeBackwardReachability::Effects - Classification classifyInstruction(SILInstruction *); + ArrayRef gens() { return uses.ends; } - bool classificationIsBarrier(Classification); + Effect effectForInstruction(SILInstruction *); + Effect effectForPhi(SILBasicBlock *); - void visitedInstruction(SILInstruction *, Classification); + /// IterativeBackwardReachability::bindBarriers::Visitor - bool checkReachableBarrier(SILInstruction *); + void visitBarrierInstruction(SILInstruction *instruction) { + barriers.instructions.push_back(instruction); + } - bool checkReachablePhiBarrier(SILBasicBlock *); + void visitBarrierPhi(SILBasicBlock *block) { barriers.phis.push_back(block); } + + void visitBarrierBlock(SILBasicBlock *block) { + barriers.blocks.push_back(block); + } }; Dataflow::Classification @@ -193,26 +189,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 +206,13 @@ 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(); + reachability.findBarriers(*this); } /// Hoist the destroy_values of %value. @@ -256,7 +244,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 +259,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 +285,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 +304,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; } diff --git a/test/SILOptimizer/lexical_destroy_hoisting.sil b/test/SILOptimizer/lexical_destroy_hoisting.sil index 16e29693cc602..9cd8bf41f56c0 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): From e68c12f251b023e80eae967c6df28f28482b7047 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Fri, 20 May 2022 19:49:58 -0700 Subject: [PATCH 3/3] [LexicalDestroyHoisting] Adopt VisitBarrierAccessScopes. Avoids hoisting destroy_values into unrelated access scopes which could introduce exclusivity violations. --- .../Utils/LexicalDestroyHoisting.cpp | 54 ++++++++++++++++ .../SILOptimizer/lexical_destroy_hoisting.sil | 64 +++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp b/lib/SILOptimizer/Utils/LexicalDestroyHoisting.cpp index 36a197220d5ea..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" @@ -112,11 +113,14 @@ struct DeinitBarriers final { 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 IterativeBackwardReachability::Effects /// Implements IterativeBackwardReachability::bindBarriers::Visitor +/// Implements VisitBarrierAccessScopes::Effects class Dataflow final { using Reachability = IterativeBackwardReachability; using Effect = Reachability::Effect; @@ -125,6 +129,7 @@ class Dataflow final { DeinitBarriers &barriers; Reachability::Result result; Reachability reachability; + SmallPtrSet barrierAccessScopes; enum class Classification { Barrier, Other }; @@ -140,18 +145,29 @@ class Dataflow final { private: friend Reachability; + friend class BarrierAccessScopeFinder; + friend class VisitBarrierAccessScopes; Classification classifyInstruction(SILInstruction *); bool classificationIsBarrier(Classification); /// IterativeBackwardReachability::Effects + /// VisitBarrierAccessScopes::Effects ArrayRef gens() { return uses.ends; } Effect effectForInstruction(SILInstruction *); Effect effectForPhi(SILBasicBlock *); + /// VisitBarrierAccessScopes::Effects + + auto localGens() { return result.localGens; } + + bool isLocalGen(SILInstruction *instruction) { + return result.localGens.contains(instruction); + } + /// IterativeBackwardReachability::bindBarriers::Visitor void visitBarrierInstruction(SILInstruction *instruction) { @@ -173,6 +189,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; } @@ -209,8 +230,41 @@ 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 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); + } + } +}; + void Dataflow::run() { reachability.initialize(); + BarrierAccessScopeFinder finder(context, *this); + finder.find(); reachability.solve(); reachability.findBarriers(*this); } diff --git a/test/SILOptimizer/lexical_destroy_hoisting.sil b/test/SILOptimizer/lexical_destroy_hoisting.sil index 9cd8bf41f56c0..8cff942dcdc00 100644 --- a/test/SILOptimizer/lexical_destroy_hoisting.sil +++ b/test/SILOptimizer/lexical_destroy_hoisting.sil @@ -460,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 }} +// =============================================================================