From 6e48e05ece89c183a2ece55d8bd242c7aa08e89e Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Fri, 15 Nov 2024 14:46:21 -0800 Subject: [PATCH 1/6] [Test] Fix shrink_borrow_scope. --- lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp | 19 ++++++------------- .../SILOptimizer/shrink_borrow_scope.unit.sil | 3 ++- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp index d9345d4ba0727..a010059507978 100644 --- a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp +++ b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp @@ -501,14 +501,11 @@ bool swift::shrinkBorrowScope( namespace swift::test { // Arguments: // - BeginBorrowInst - the introducer for the scope to shrink -// - bool - the expected return value of shrinkBorrowScope -// - variadic list of values consisting of the copies expected to be rewritten // Dumps: // - DELETED: <> static FunctionTest ShrinkBorrowScopeTest( - "shrink-borrow-scope", [](auto &function, auto &arguments, auto &test) { + "shrink_borrow_scope", [](auto &function, auto &arguments, auto &test) { auto instruction = arguments.takeValue(); - auto expected = arguments.takeBool(); auto *bbi = cast(instruction); auto *analysis = test.template getAnalysis(); SmallVector modifiedCopyValueInsts; @@ -516,20 +513,16 @@ static FunctionTest ShrinkBorrowScopeTest( InstModCallbacks().onDelete([&](auto *instruction) { llvm::outs() << "DELETED:\n"; instruction->print(llvm::outs()); + instruction->eraseFromParent(); + })); auto shrunk = shrinkBorrowScope(*bbi, deleter, analysis, modifiedCopyValueInsts); - unsigned index = 0; + auto *shrunkString = shrunk ? "shrunk" : "did not shrink"; + llvm::outs() << "Result: " << shrunkString << "\n"; + llvm::outs() << "Rewrote the following copies:\n"; for (auto *cvi : modifiedCopyValueInsts) { - auto expectedCopy = arguments.takeValue(); - llvm::outs() << "rewritten copy " << index << ":\n"; - llvm::outs() << "expected:\n"; - expectedCopy->print(llvm::outs()); - llvm::outs() << "got:\n"; cvi->print(llvm::outs()); - assert(cvi == expectedCopy); - ++index; } - assert(expected == shrunk && "didn't shrink expectedly!?"); }); } // namespace swift::test diff --git a/test/SILOptimizer/shrink_borrow_scope.unit.sil b/test/SILOptimizer/shrink_borrow_scope.unit.sil index 4372269cb6400..2f934d28d0fb3 100644 --- a/test/SILOptimizer/shrink_borrow_scope.unit.sil +++ b/test/SILOptimizer/shrink_borrow_scope.unit.sil @@ -11,10 +11,11 @@ sil [ossa] @callee_guaranteed: $@convention(thin) (@guaranteed C) -> () // CHECK-LABEL: begin running test 1 of {{[^,]+}} on nohoist_over_rewritten_copy // CHECK-NOT: DELETED: // CHECK-NOT: end_borrow +// CHECK: Result: did not shrink // CHECK-LABEL: end running test 1 of {{[^,]+}} on nohoist_over_rewritten_copy sil [ossa] @nohoist_over_rewritten_copy : $@convention(thin) () -> (@owned C, @owned C) { entry: - specify_test "shrink-borrow-scope @trace true @trace[1]" + specify_test "shrink_borrow_scope %lifetime" %get_owned_c = function_ref @get_owned_c : $@convention(thin) () -> (@owned C) %instance = apply %get_owned_c() : $@convention(thin) () -> (@owned C) %lifetime = begin_borrow [lexical] %instance : $C From 71b19e743f8f03865f0f849e0fd50c2d4e3faaf5 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 19 Nov 2024 17:44:12 -0800 Subject: [PATCH 2/6] [Gardening] Test: Detypo'd comment abbreviation. --- test/SILOptimizer/hoist_destroy_addr.sil | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/SILOptimizer/hoist_destroy_addr.sil b/test/SILOptimizer/hoist_destroy_addr.sil index ec19968d56068..09e644cb6205e 100644 --- a/test/SILOptimizer/hoist_destroy_addr.sil +++ b/test/SILOptimizer/hoist_destroy_addr.sil @@ -1174,7 +1174,7 @@ entry(%addr : $*X): return %retval : $() } -// Even though the apply of %empty is not a deinit barrier (c.f. +// Even though the apply of %empty is not a deinit barrier (cf. // hoist_over_apply_of_non_barrier_fn), verify that the destroy_addr is not // hoisted, because MoS is move-only. // CHECK-LABEL: sil [ossa] @dont_move_destroy_addr_of_moveonly_struct : {{.*}} { From 2d1bdb84e6cbd0a2049f273005dbc6fcfe4c9918 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 19 Nov 2024 17:55:34 -0800 Subject: [PATCH 3/6] [Gardening] Add missing word in comment. --- lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp index a010059507978..da5909e5dcdb2 100644 --- a/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp +++ b/lib/SILOptimizer/Utils/ShrinkBorrowScope.cpp @@ -296,8 +296,8 @@ Dataflow::Effect Dataflow::effectForPhi(SILBasicBlock *block) { } /// 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. +/// access scopes they end contain barriers to hoisting. Hoisting end_borrows +/// into such access scopes could introduce exclusivity violations. /// /// Implements BarrierAccessScopeFinder::Visitor class BarrierAccessScopeFinder final { From 5c5f06e87193f79c235661c9d099891d0ac234bc Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 2 Dec 2024 08:20:30 -0800 Subject: [PATCH 4/6] [Gardening] BarrierAccessScopes: Corrected comment. --- .../swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h index aa30d4d788205..091299f324c31 100644 --- a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h +++ b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h @@ -259,8 +259,9 @@ class VisitBarrierAccessScopes { } } // If any of this block's predecessors haven't already been visited, it - // means that they aren't in the region and consequently this block is a - // barrier block. + // means EITHER that those predecessors aren't in the region OR that a + // barrier was encountered when visiting some (iterative) successor of that + // predecessor. Either way, this block is a barrier block as a result. if (llvm::any_of(block->getSuccessorBlocks(), [&](SILBasicBlock *block) { return !visited.contains(block); })) { From fa126d6d4c18ae1156b442cb84dcfbcb4d319113 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 2 Dec 2024 08:22:16 -0800 Subject: [PATCH 5/6] [NFC] BarrierAccessScopes: Renamed function. --- .../SILOptimizer/Analysis/VisitBarrierAccessScopes.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h index 091299f324c31..385caa00dbfb0 100644 --- a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h +++ b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h @@ -156,7 +156,7 @@ class VisitBarrierAccessScopes { // whose successors are in the region) blocks in the region only the top // of which was already visited. Either way, the instructions between the // local gen and its kill have not yet been visited. Visit them now. - auto foundLocalKill = visitBlockFromGenUntilBegin(instruction); + auto foundLocalKill = visitBlockFromGenThroughKill(instruction); assert(foundLocalKill && "local gen without local kill?!"); (void)foundLocalKill; } @@ -165,13 +165,14 @@ class VisitBarrierAccessScopes { private: /// Entry points for visiting: they visit increasingly large portions of a /// block. - /// - visitBlockFromGenUntilBegin: Instructions and phi until a kill. + /// - visitBlockFromGenThroughKill: Instructions and phi through the first + /// kill. /// - visitBlockFromGen: Instructions, phi, and begin. /// - visitBlock: End, instructions, phi, and begin. /// Visit instructions and phis starting from the specified gen until a kill /// is found. - bool visitBlockFromGenUntilBegin(SILInstruction *from) { + bool visitBlockFromGenThroughKill(SILInstruction *from) { assert(effects.effectForInstruction(from) == Effects::Effect::Gen()); for (auto *instruction = from; instruction; instruction = instruction->getPreviousInstruction()) { @@ -198,7 +199,7 @@ class VisitBarrierAccessScopes { [&](SILBasicBlock *successor) { return visited.contains(successor); })); visited.insert(block); - bool foundLocalKill = visitBlockFromGenUntilBegin(from); + bool foundLocalKill = visitBlockFromGenThroughKill(from); assert(!foundLocalKill && "found local kill for non-local gen?!"); (void)foundLocalKill; visitBlockBegin(block); From f79def4ceedaefd63a37bc64a19a7408c6469c73 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 2 Dec 2024 12:21:03 -0800 Subject: [PATCH 6/6] [BarrierAccessScopes] Handle found gen locality. As the utility runs, new gens may become local: as access scopes are determined to contain deinit barriers, their `end_access` instructions become kills; if such an `end_access` occurs in the same block above an initially-non-local gen, that gen is now local. Previously, it was asserted that initially-non-local gens would not encounter when visiting the block backwards from that gen. Iteration would also _stop_ at the discovered kill, if any. As described above, the assertion was incorrect. Stopping at the discovered kill was also incorrect. It's necessary to continue walking the block after finding such a new kill because the book-keeping the utility does for which access scopes contain barriers. Concretely, there are two cases: (1) It may contain another `end_access` and above it a deinit barrier which must result in that second scope becoming a deinit barrier. (2) Some of its predecessors may be in the region, all the access scopes which are open at the begin of this block must be unioned into the set of scopes open at each predecessors' end, and more such access scopes may be discovered above the just-visited `end_access`. Here, both the assertion failure and the early bailout are fixed by walking from the indicated initially-non-local gen backwards over the entire block, regardless of whether a kill was encountered. If a kill is encountered, it is asserted that the kill is an `end_access` to account for the case described above. rdar://139840307 --- .../Analysis/VisitBarrierAccessScopes.h | 18 +++++++-- .../SILOptimizer/shrink_borrow_scope.unit.sil | 38 +++++++++++++++++++ validation-test/SILOptimizer/gh77693.swift | 17 +++++++++ 3 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 validation-test/SILOptimizer/gh77693.swift diff --git a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h index 385caa00dbfb0..0929128480ef7 100644 --- a/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h +++ b/include/swift/SILOptimizer/Analysis/VisitBarrierAccessScopes.h @@ -199,9 +199,21 @@ class VisitBarrierAccessScopes { [&](SILBasicBlock *successor) { return visited.contains(successor); })); visited.insert(block); - bool foundLocalKill = visitBlockFromGenThroughKill(from); - assert(!foundLocalKill && "found local kill for non-local gen?!"); - (void)foundLocalKill; + for (auto *instruction = from; instruction; + instruction = instruction->getPreviousInstruction()) { + if (visitInstruction(instruction)) { + // New kills are incrementally added as access scopes are determined to + // be barriers. For this reason, gens may newly be discovered to be + // local. This can only happen when the kill which makes the gen local + // ends an access scope (i.e. is an end_access). + assert(isa(instruction) && + "found preexisting local kill for initially-non-local gen?!"); + // Even so, the remainder of the block must still be visited. + } + } + if (block->hasPhi()) { + visitPhi(block); + } visitBlockBegin(block); } diff --git a/test/SILOptimizer/shrink_borrow_scope.unit.sil b/test/SILOptimizer/shrink_borrow_scope.unit.sil index 2f934d28d0fb3..f3544ada8cdbb 100644 --- a/test/SILOptimizer/shrink_borrow_scope.unit.sil +++ b/test/SILOptimizer/shrink_borrow_scope.unit.sil @@ -5,8 +5,12 @@ import Swift class C {} +struct X {} + sil [ossa] @get_owned_c : $@convention(thin) () -> (@owned C) sil [ossa] @callee_guaranteed: $@convention(thin) (@guaranteed C) -> () +sil [ossa] @barrier : $@convention(thin) () -> () + // CHECK-LABEL: begin running test 1 of {{[^,]+}} on nohoist_over_rewritten_copy // CHECK-NOT: DELETED: @@ -29,3 +33,37 @@ entry: return %retval : $(C, C) } +// CHECK-LABEL: begin running test {{.*}} on nohoist_over_access_scope_barrier_1 +// CHECK: Result: did not shrink +// CHECK-LABEL: end running test {{.*}} on nohoist_over_access_scope_barrier_1 +sil [ossa] @nohoist_over_access_scope_barrier_1 : $@convention(thin) (@guaranteed C, @inout X) -> () { +entry(%c : @guaranteed $C, %addr : $*X): + %borrow = begin_borrow %c : $C + specify_test "shrink_borrow_scope %borrow" + %access = begin_access [modify] [dynamic] %addr : $*X + cond_br undef, kill_introducer, newly_local_gen + +kill_introducer: + cond_br undef, left, right + +left: + %barrier = function_ref @barrier : $@convention(thin) () -> () + apply %barrier() : $@convention(thin) () -> () + end_access %access : $*X + end_borrow %borrow : $C + br exit + +right: + end_access %access : $*X + end_borrow %borrow : $C + br exit + +newly_local_gen: + end_access %access : $*X + end_borrow %borrow : $C + br exit + +exit: + %retval = tuple () + return %retval : $() +} diff --git a/validation-test/SILOptimizer/gh77693.swift b/validation-test/SILOptimizer/gh77693.swift new file mode 100644 index 0000000000000..c333279f2a527 --- /dev/null +++ b/validation-test/SILOptimizer/gh77693.swift @@ -0,0 +1,17 @@ +// RUN: %target-swift-frontend -c -O %s + +protocol P {} +enum E { + case c(any P) + var d: [String:String] { [:] } +} + +final class C { + var o: [String:String]? +} + +func f(_ e: E?) { + if let e { + C().o?.merge(e.d) { c, _ in c } + } +}