From 646dc136fcfadc647e5ed6ef955eba69be1e4e03 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 7 Dec 2021 16:49:53 -0800 Subject: [PATCH 1/5] [CopyPropagation] Note ShrinkBorrowScope changes. Use the result returned from shrinkgBorrowScope to update the changed flag. --- lib/SILOptimizer/Transforms/CopyPropagation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/SILOptimizer/Transforms/CopyPropagation.cpp b/lib/SILOptimizer/Transforms/CopyPropagation.cpp index 20b9dee700a30..b5c5646d85449 100644 --- a/lib/SILOptimizer/Transforms/CopyPropagation.cpp +++ b/lib/SILOptimizer/Transforms/CopyPropagation.cpp @@ -438,7 +438,7 @@ void CopyPropagation::run() { // blocks and pushing begin_borrows as we see them and then popping them // off the end will result in shrinking inner borrow scopes first. while (auto *bbi = beginBorrowsToShrink.pop()) { - shrinkBorrowScope(bbi, deleter); + changed |= shrinkBorrowScope(bbi, deleter); } // canonicalizer performs all modifications through deleter's callbacks, so we From 5e8f369d065ff320123f203a3ed03d71b6b1731b Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 7 Dec 2021 16:52:01 -0800 Subject: [PATCH 2/5] [Gardening] Merged two conditions. --- lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp index 7c72a534a7a97..59c0dbe076e15 100644 --- a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp +++ b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp @@ -49,9 +49,8 @@ bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) { // Lexical borrow scopes must remain in order to ensure that value lifetimes // are not observably shortened. - if (bbi->isLexical()) { - if (!isRedundantLexicalBeginBorrow(bbi)) - return false; + if (bbi->isLexical() && !isRedundantLexicalBeginBorrow(bbi)) { + return false; } auto kind = bbi->getOperand().getOwnershipKind(); From 75b351eda0d8e088167ff305d7156d07bf8b0147 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 7 Dec 2021 16:52:53 -0800 Subject: [PATCH 3/5] [Gardening] Updated comment. --- lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp index 59c0dbe076e15..625dc4cb608ae 100644 --- a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp +++ b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp @@ -47,8 +47,8 @@ bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) { if (!ctx.shouldPerform(ARCTransformKind::RedundantBorrowScopeElimPeephole)) return false; - // Lexical borrow scopes must remain in order to ensure that value lifetimes - // are not observably shortened. + // Non-redundant, lexical borrow scopes must remain in order to ensure that + // value lifetimes are not observably shortened. if (bbi->isLexical() && !isRedundantLexicalBeginBorrow(bbi)) { return false; } From f0bd5839a2cbbd2f0850c8832c3f18b458683537 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 7 Dec 2021 16:55:15 -0800 Subject: [PATCH 4/5] [OwnershipUtils] Extracted utility for common use. Promoted isRedundantLexicalBeginBorrow function to OwnershipUtils so that it can be used in more than just SemanticARCOpts. --- include/swift/SIL/OwnershipUtils.h | 8 ++++++++ lib/SIL/Utils/OwnershipUtils.cpp | 18 +++++++++++++++++ .../SemanticARC/BorrowScopeOpts.cpp | 20 +------------------ 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/include/swift/SIL/OwnershipUtils.h b/include/swift/SIL/OwnershipUtils.h index b9c0d68326bd8..2cedc9bedcf60 100644 --- a/include/swift/SIL/OwnershipUtils.h +++ b/include/swift/SIL/OwnershipUtils.h @@ -1206,6 +1206,14 @@ void visitTransitiveEndBorrows( BorrowedValue beginBorrow, function_ref visitEndBorrow); +/// Whether the specified lexical begin_borrow instruction is nested. +/// +/// A begin_borrow [lexical] is nested if the borrowed value's lifetime is +/// guaranteed by another lexical scope. That happens if: +/// - the value is a guaranteed argument to the function +/// - the value is itself a begin_borrow [lexical] +bool isNestedLexicalBeginBorrow(BeginBorrowInst *bbi); + } // namespace swift #endif diff --git a/lib/SIL/Utils/OwnershipUtils.cpp b/lib/SIL/Utils/OwnershipUtils.cpp index abf65317d9f79..6c60b718d8bac 100644 --- a/lib/SIL/Utils/OwnershipUtils.cpp +++ b/lib/SIL/Utils/OwnershipUtils.cpp @@ -1487,3 +1487,21 @@ void swift::visitTransitiveEndBorrows( } } } + +/// Whether the specified lexical begin_borrow instruction is nested. +/// +/// A begin_borrow [lexical] is nested if the borrowed value's lifetime is +/// guaranteed by another lexical scope. That happens if: +/// - the value is a guaranteed argument to the function +/// - the value is itself a begin_borrow [lexical] +bool swift::isNestedLexicalBeginBorrow(BeginBorrowInst *bbi) { + assert(bbi->isLexical()); + auto value = bbi->getOperand(); + if (auto *outerBBI = dyn_cast(value)) { + return outerBBI->isLexical(); + } + if (auto *arg = dyn_cast(value)) { + return arg->getOwnershipKind() == OwnershipKind::Guaranteed; + } + return false; +} diff --git a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp index 625dc4cb608ae..2ff6f9cdeeb41 100644 --- a/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp +++ b/lib/SILOptimizer/SemanticARC/BorrowScopeOpts.cpp @@ -24,24 +24,6 @@ using namespace swift; using namespace swift::semanticarc; -/// Whether the provided lexical begin_borrow instruction is redundant. -/// -/// A begin_borrow [lexical] is redundant if the borrowed value's lifetime is -/// otherwise guaranteed. That happens if: -/// - the value is a guaranteed argument to the function -/// - the value is itself a begin_borrow [lexical] -static bool isRedundantLexicalBeginBorrow(BeginBorrowInst *bbi) { - assert(bbi->isLexical()); - auto value = bbi->getOperand(); - if (auto *outerBBI = dyn_cast(value)) { - return outerBBI->isLexical(); - } - if (auto *arg = dyn_cast(value)) { - return arg->getOwnershipKind() == OwnershipKind::Guaranteed; - } - return false; -} - bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) { // Quickly check if we are supposed to perform this transformation. if (!ctx.shouldPerform(ARCTransformKind::RedundantBorrowScopeElimPeephole)) @@ -49,7 +31,7 @@ bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) { // Non-redundant, lexical borrow scopes must remain in order to ensure that // value lifetimes are not observably shortened. - if (bbi->isLexical() && !isRedundantLexicalBeginBorrow(bbi)) { + if (bbi->isLexical() && !isNestedLexicalBeginBorrow(bbi)) { return false; } From ac50bb7a7ccc02c63ee058d23c883587bba99a97 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 7 Dec 2021 16:59:54 -0800 Subject: [PATCH 5/5] [CanonicalizeInst] Remove redundant lexical bbis. Previously, CanonicalizeInstruction::eliminateSimpleBorrows bailed out when encountering any any [lexical] begin_borrow. While generally such borrow scopes must be preserved, they may be removed if they are redundant (when the borrowed value is guaranteed by another means: an outer lexical borrow scope or a guaranteed function argument). Here, removing such redundant lexical borrow scopes is enabled. --- lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp b/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp index 2959caf8afe14..22c9f87ff1843 100644 --- a/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp +++ b/lib/SILOptimizer/Utils/CanonicalizeInstruction.cpp @@ -445,9 +445,13 @@ static SILBasicBlock::iterator eliminateSimpleBorrows(BeginBorrowInst *bbi, CanonicalizeInstruction &pass) { auto next = std::next(bbi->getIterator()); - // Never eliminate lexical borrow scopes. They must be kept to ensure that - // value lifetimes aren't observably shortened. - if (bbi->isLexical()) + // Lexical borrow scopes can only be eliminated under certain circumstances: + // (1) They can never be eliminated if the module is in the raw stage, because + // they may be needed for diagnostic. + // (2) They can never be eliminated if there is no enclosing lexical scope + // which guarantees the lifetime of the value. + if (bbi->isLexical() && (bbi->getModule().getStage() == SILStage::Raw || + !isNestedLexicalBeginBorrow(bbi))) return next; // We know that our borrow is completely within the lifetime of its base value