From a17feaa4c8a728d79bb9b96fb76712957dcfd720 Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Thu, 12 Oct 2023 13:12:24 -0700 Subject: [PATCH 1/7] SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes. When rewriting uses of a noncopyable value, the move-only checker failed to take into account the scope of borrowing uses when establishing the final lifetimes of values. One way this manifested was when borrowed values get reabstracted from value to in-memory representations, using a store_borrow instruction, the lifetime of the original borrow would be ended immediately after the store_borrow begins rather than after the matching end_borrow. Fix this by, first, changing `store_borrow` to be treated as a borrowing use of its source rather than an interior-pointer use; this should be more accurate overall since `store_borrow` borrows the entire source value for a well-scoped duration balanced by `end_borrow` instructions. That done, change MoveOnlyBorrowToDestructureUtils so that when it sees a borrow use, it ends the borrow at the end(s) of the use's borrow scope, instead of immediately after the beginning of the use. --- include/swift/SIL/OwnershipUtils.h | 15 ++-- lib/SIL/IR/OperandOwnership.cpp | 2 +- lib/SIL/Utils/OwnershipUtils.cpp | 20 +++++ .../MoveOnlyBorrowToDestructureUtils.cpp | 87 +++++++++++++++---- .../SILCombinerBuiltinVisitors.cpp | 1 - .../Utils/CanonicalizeOSSALifetime.cpp | 22 +++-- test/SILOptimizer/OSLogMandatoryOptTest.swift | 27 ++---- .../moveonly_resilient_property_reader.swift | 15 ++++ 8 files changed, 138 insertions(+), 51 deletions(-) create mode 100644 test/SILOptimizer/moveonly_resilient_property_reader.swift diff --git a/include/swift/SIL/OwnershipUtils.h b/include/swift/SIL/OwnershipUtils.h index 87f76b4d47525..b9ba361799e26 100644 --- a/include/swift/SIL/OwnershipUtils.h +++ b/include/swift/SIL/OwnershipUtils.h @@ -254,6 +254,7 @@ class BorrowingOperandKind { enum Kind : uint8_t { Invalid = 0, BeginBorrow, + StoreBorrow, BeginApply, Branch, Apply, @@ -277,6 +278,8 @@ class BorrowingOperandKind { return Kind::Invalid; case SILInstructionKind::BeginBorrowInst: return Kind::BeginBorrow; + case SILInstructionKind::StoreBorrowInst: + return Kind::StoreBorrow; case SILInstructionKind::BeginApplyInst: return Kind::BeginApply; case SILInstructionKind::BranchInst: @@ -386,6 +389,7 @@ struct BorrowingOperand { case BorrowingOperandKind::Invalid: llvm_unreachable("Using invalid case?!"); case BorrowingOperandKind::BeginBorrow: + case BorrowingOperandKind::StoreBorrow: case BorrowingOperandKind::BeginApply: case BorrowingOperandKind::Apply: case BorrowingOperandKind::TryApply: @@ -418,6 +422,7 @@ struct BorrowingOperand { case BorrowingOperandKind::BeginBorrow: case BorrowingOperandKind::Branch: return true; + case BorrowingOperandKind::StoreBorrow: case BorrowingOperandKind::BeginApply: case BorrowingOperandKind::Apply: case BorrowingOperandKind::TryApply: @@ -790,7 +795,6 @@ class InteriorPointerOperandKind { RefTailAddr, OpenExistentialBox, ProjectBox, - StoreBorrow, }; private: @@ -819,8 +823,6 @@ class InteriorPointerOperandKind { return Kind::OpenExistentialBox; case SILInstructionKind::ProjectBoxInst: return Kind::ProjectBox; - case SILInstructionKind::StoreBorrowInst: - return Kind::StoreBorrow; } } @@ -839,8 +841,6 @@ class InteriorPointerOperandKind { return Kind::OpenExistentialBox; case ValueKind::ProjectBoxInst: return Kind::ProjectBox; - case ValueKind::StoreBorrowInst: - return Kind::StoreBorrow; } } @@ -897,8 +897,7 @@ struct InteriorPointerOperand { case InteriorPointerOperandKind::RefElementAddr: case InteriorPointerOperandKind::RefTailAddr: case InteriorPointerOperandKind::OpenExistentialBox: - case InteriorPointerOperandKind::ProjectBox: - case InteriorPointerOperandKind::StoreBorrow: { + case InteriorPointerOperandKind::ProjectBox: { // Ok, we have a valid instruction. Return the relevant operand. auto *op = &cast(resultValue)->getAllOperands()[0]; @@ -941,8 +940,6 @@ struct InteriorPointerOperand { return cast(operand->getUser()); case InteriorPointerOperandKind::ProjectBox: return cast(operand->getUser()); - case InteriorPointerOperandKind::StoreBorrow: - return cast(operand->getUser()); } llvm_unreachable("Covered switch isn't covered?!"); } diff --git a/lib/SIL/IR/OperandOwnership.cpp b/lib/SIL/IR/OperandOwnership.cpp index 215d071c35497..31c947ead6388 100644 --- a/lib/SIL/IR/OperandOwnership.cpp +++ b/lib/SIL/IR/OperandOwnership.cpp @@ -452,7 +452,7 @@ OperandOwnership OperandOwnershipClassifier::visitBranchInst(BranchInst *bi) { OperandOwnership OperandOwnershipClassifier::visitStoreBorrowInst(StoreBorrowInst *i) { if (getValue() == i->getSrc()) { - return OperandOwnership::InteriorPointer; + return OperandOwnership::Borrow; } return OperandOwnership::TrivialUse; } diff --git a/lib/SIL/Utils/OwnershipUtils.cpp b/lib/SIL/Utils/OwnershipUtils.cpp index da1b24720ea69..9c81e1d4aa171 100644 --- a/lib/SIL/Utils/OwnershipUtils.cpp +++ b/lib/SIL/Utils/OwnershipUtils.cpp @@ -600,6 +600,9 @@ void BorrowingOperandKind::print(llvm::raw_ostream &os) const { case Kind::BeginBorrow: os << "BeginBorrow"; return; + case Kind::StoreBorrow: + os << "StoreBorrow"; + return; case Kind::BeginApply: os << "BeginApply"; return; @@ -649,6 +652,7 @@ bool BorrowingOperand::hasEmptyRequiredEndingUses() const { case BorrowingOperandKind::Invalid: llvm_unreachable("Using invalid case"); case BorrowingOperandKind::BeginBorrow: + case BorrowingOperandKind::StoreBorrow: case BorrowingOperandKind::BeginApply: case BorrowingOperandKind::BeginAsyncLet: case BorrowingOperandKind::PartialApplyStack: { @@ -687,6 +691,20 @@ bool BorrowingOperand::visitScopeEndingUses( // false. return !deadBorrow; } + case BorrowingOperandKind::StoreBorrow: { + bool deadBorrow = true; + for (auto *use : cast(op->getUser())->getUses()) { + if (isa(use->getUser())) { + deadBorrow = false; + if (!func(use)) + return false; + } + } + // FIXME: special case for dead borrows. This is dangerous because clients + // only expect visitScopeEndingUses to return false if the visitor returned + // false. + return !deadBorrow; + } case BorrowingOperandKind::BeginApply: { bool deadApply = true; auto *user = cast(op->getUser()); @@ -763,6 +781,7 @@ BorrowedValue BorrowingOperand::getBorrowIntroducingUserResult() const { case BorrowingOperandKind::Yield: case BorrowingOperandKind::PartialApplyStack: case BorrowingOperandKind::BeginAsyncLet: + case BorrowingOperandKind::StoreBorrow: return BorrowedValue(); case BorrowingOperandKind::BeginBorrow: { @@ -792,6 +811,7 @@ SILValue BorrowingOperand::getScopeIntroducingUserResult() { case BorrowingOperandKind::BeginAsyncLet: case BorrowingOperandKind::PartialApplyStack: case BorrowingOperandKind::BeginBorrow: + case BorrowingOperandKind::StoreBorrow: return cast(op->getUser()); case BorrowingOperandKind::BeginApply: diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp index a1b23b6025f39..5bec8d043e4d8 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp @@ -437,6 +437,17 @@ bool Implementation::gatherUses(SILValue value) { false /*is lifetime ending*/}}); liveness.updateForUse(nextUse->getUser(), *leafRange, false /*is lifetime ending*/); + // The liveness extends to the scope-ending uses of the borrow. + BorrowingOperand(nextUse).visitScopeEndingUses([&](Operand *end) -> bool { + if (end->getOperandOwnership() == OperandOwnership::Reborrow) { + return false; + } + LLVM_DEBUG(llvm::dbgs() << " ++ Scope-ending use: "; + end->getUser()->print(llvm::dbgs())); + liveness.updateForUse(end->getUser(), *leafRange, + false /*is lifetime ending*/); + return true; + }); instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse); continue; @@ -1001,6 +1012,48 @@ dumpIntervalMap(IntervalMapAllocator::Map &map) { } #endif +// Helper to insert end_borrows after the end of a non-consuming use. If the +// use is momentary, one end_borrow is inserted after the use. If it is an +// interior pointer projection or nested borrow, then end_borrows are inserted +// after every scope-ending instruction for the use. +static void insertEndBorrowsForNonConsumingUse(Operand *op, + SILValue borrow) { + if (auto iOp = InteriorPointerOperand::get(op)) { + LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after interior pointer scope:\n" + " "; + op->getUser()->print(llvm::dbgs())); + iOp.visitBaseValueScopeEndingUses([&](Operand *endScope) -> bool { + auto *endScopeInst = endScope->getUser(); + LLVM_DEBUG(llvm::dbgs() << " "; + endScopeInst->print(llvm::dbgs())); + SILBuilderWithScope endBuilder(endScopeInst); + endBuilder.createEndBorrow(getSafeLoc(endScopeInst), borrow); + return true; + }); + } else if (auto bOp = BorrowingOperand(op)) { + LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after borrow scope:\n" + " "; + op->getUser()->print(llvm::dbgs())); + bOp.visitScopeEndingUses([&](Operand *endScope) -> bool { + auto *endScopeInst = endScope->getUser(); + LLVM_DEBUG(llvm::dbgs() << " "; + endScopeInst->print(llvm::dbgs())); + auto afterScopeInst = endScopeInst->getNextInstruction(); + SILBuilderWithScope endBuilder(afterScopeInst); + endBuilder.createEndBorrow(getSafeLoc(afterScopeInst), + borrow); + return true; + }); + } else { + auto *nextInst = op->getUser()->getNextInstruction(); + LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after momentary use at: "; + nextInst->print(llvm::dbgs())); + SILBuilderWithScope endBuilder(nextInst); + endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow); + } + +} + void Implementation::rewriteUses(InstructionDeleter *deleter) { blocksToUses.setFrozen(); @@ -1129,6 +1182,8 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) { } // Otherwise, we need to insert a borrow. + LLVM_DEBUG(llvm::dbgs() << " Inserting borrow for: "; + inst->print(llvm::dbgs())); SILBuilderWithScope borrowBuilder(inst); SILValue borrow = borrowBuilder.createBeginBorrow(getSafeLoc(inst), first); @@ -1138,21 +1193,10 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) { borrowBuilder.createGuaranteedMoveOnlyWrapperToCopyableValue( getSafeLoc(inst), innerValue); } + + insertEndBorrowsForNonConsumingUse(&operand, borrow); - if (auto op = InteriorPointerOperand::get(&operand)) { - op.visitBaseValueScopeEndingUses([&](Operand *endScope) -> bool { - auto *endScopeInst = endScope->getUser(); - SILBuilderWithScope endBuilder(endScopeInst); - endBuilder.createEndBorrow(getSafeLoc(endScopeInst), borrow); - return true; - }); - } else { - auto *nextInst = inst->getNextInstruction(); - SILBuilderWithScope endBuilder(nextInst); - endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow); - } - - // NOTE: This needs to be /after/the interior pointer operand usage + // NOTE: This needs to be /after/ the interior pointer operand usage // above so that we can use the end scope of our interior pointer base // value. // NOTE: oldInst may be nullptr if our operand is a SILArgument @@ -1245,9 +1289,7 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) { } } - auto *nextInst = inst->getNextInstruction(); - SILBuilderWithScope endBuilder(nextInst); - endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow); + insertEndBorrowsForNonConsumingUse(&operand, borrow); continue; } @@ -1333,6 +1375,13 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) { } void Implementation::cleanup() { + LLVM_DEBUG(llvm::dbgs() + << "Performing BorrowToDestructureTransform::cleanup()!\n"); + SWIFT_DEFER { + LLVM_DEBUG(llvm::dbgs() << "Function after cleanup!\n"; + getMarkedValue()->getFunction()->dump()); + }; + // Then add destroys for any destructure elements that we inserted that we did // not actually completely consume. auto *fn = getMarkedValue()->getFunction(); @@ -1793,11 +1842,15 @@ bool BorrowToDestructureTransform::transform() { // Then clean up all of our borrows/copies/struct_extracts which no longer // have any uses... { + LLVM_DEBUG(llvm::dbgs() << "Deleting dead instructions!\n"); + InstructionDeleter deleter; while (!borrowWorklist.empty()) { deleter.recursivelyForceDeleteUsersAndFixLifetimes( borrowWorklist.pop_back_val()); } + LLVM_DEBUG(llvm::dbgs() << "Function after deletion!\n"; + impl.getMarkedValue()->getFunction()->dump()); } return true; diff --git a/lib/SILOptimizer/SILCombiner/SILCombinerBuiltinVisitors.cpp b/lib/SILOptimizer/SILCombiner/SILCombinerBuiltinVisitors.cpp index 0c81cf4ffa754..b5d51a2437bc0 100644 --- a/lib/SILOptimizer/SILCombiner/SILCombinerBuiltinVisitors.cpp +++ b/lib/SILOptimizer/SILCombiner/SILCombinerBuiltinVisitors.cpp @@ -162,7 +162,6 @@ SILCombiner::optimizeBuiltinCOWBufferForReadingOSSA(BuiltinInst *bi) { return; case InteriorPointerOperandKind::OpenExistentialBox: case InteriorPointerOperandKind::ProjectBox: - case InteriorPointerOperandKind::StoreBorrow: // Can not mark this immutable. return; } diff --git a/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp b/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp index 531f8dd0d9c2c..51aca582f0eac 100644 --- a/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp +++ b/lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp @@ -129,6 +129,8 @@ static bool isDestroyOfCopyOf(SILInstruction *instruction, SILValue def) { //===----------------------------------------------------------------------===// bool CanonicalizeOSSALifetime::computeCanonicalLiveness() { + LLVM_DEBUG(llvm::dbgs() << "Computing canonical liveness from:\n"; + getCurrentDef()->print(llvm::dbgs())); defUseWorklist.initialize(getCurrentDef()); // Only the first level of reborrows need to be consider. All nested inner // adjacent reborrows and phis are encapsulated within their lifetimes. @@ -140,7 +142,13 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() { }); } while (SILValue value = defUseWorklist.pop()) { + LLVM_DEBUG(llvm::dbgs() << " Uses of value:\n"; + value->print(llvm::dbgs())); + for (Operand *use : value->getUses()) { + LLVM_DEBUG(llvm::dbgs() << " Use:\n"; + use->getUser()->print(llvm::dbgs())); + auto *user = use->getUser(); // Recurse through copies. if (auto *copy = dyn_cast(user)) { @@ -169,6 +177,7 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() { // escape. Is it legal to canonicalize ForwardingUnowned? case OperandOwnership::ForwardingUnowned: case OperandOwnership::PointerEscape: + LLVM_DEBUG(llvm::dbgs() << " Value escaped! Giving up\n"); return false; case OperandOwnership::InstantaneousUse: case OperandOwnership::UnownedInstantaneousUse: @@ -197,7 +206,8 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() { break; case OperandOwnership::Borrow: if (liveness->updateForBorrowingOperand(use) - != InnerBorrowKind::Contained) { + != InnerBorrowKind::Contained) { + LLVM_DEBUG(llvm::dbgs() << " Inner borrow can't be contained! Giving up\n"); return false; } break; @@ -1154,11 +1164,13 @@ void CanonicalizeOSSALifetime::rewriteCopies() { //===----------------------------------------------------------------------===// bool CanonicalizeOSSALifetime::computeLiveness() { - if (currentDef->getOwnershipKind() != OwnershipKind::Owned) - return false; - LLVM_DEBUG(llvm::dbgs() << " Canonicalizing: " << currentDef); + if (currentDef->getOwnershipKind() != OwnershipKind::Owned) { + LLVM_DEBUG(llvm::dbgs() << " not owned, never mind\n"); + return false; + } + // Note: There is no need to register callbacks with this utility. 'onDelete' // is the only one in use to handle dangling pointers, which could be done // instead be registering a temporary handler with the pass. Canonicalization @@ -1175,7 +1187,7 @@ bool CanonicalizeOSSALifetime::computeLiveness() { // Step 1: compute liveness if (!computeCanonicalLiveness()) { - LLVM_DEBUG(llvm::errs() << "Failed to compute canonical liveness?!\n"); + LLVM_DEBUG(llvm::dbgs() << "Failed to compute canonical liveness?!\n"); clear(); return false; } diff --git a/test/SILOptimizer/OSLogMandatoryOptTest.swift b/test/SILOptimizer/OSLogMandatoryOptTest.swift index d2f6316e2d8cd..345bfa81f1342 100644 --- a/test/SILOptimizer/OSLogMandatoryOptTest.swift +++ b/test/SILOptimizer/OSLogMandatoryOptTest.swift @@ -46,9 +46,8 @@ func testSimpleInterpolation() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // We need to wade through some borrows and copy values here. - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -93,8 +92,7 @@ func testInterpolationWithFormatOptions() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -141,8 +139,7 @@ func testInterpolationWithFormatOptionsAndPrivacy() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -195,9 +192,8 @@ func testInterpolationWithMultipleArguments() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAYADDR]] = alloc_stack $Array<(inout UnsafeMutablePointer, inout Optional>, inout Optional>) -> ()> - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -246,8 +242,7 @@ func testLogMessageWithoutData() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to {{.*}} - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to {{.*}} // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[ARGSARRAY:%[0-9]+]] // CHECK-DAG: ([[ARGSARRAY]], {{%.*}}) = destructure_tuple [[ARRAYINITRES:%[0-9]+]] @@ -319,8 +314,7 @@ func testMessageWithTooManyArguments() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -404,8 +398,7 @@ func testDynamicStringArguments() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -457,8 +450,7 @@ func testNSObjectInterpolation() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF @@ -505,8 +497,7 @@ func testDoubleInterpolation() { // CHECK-DAG: [[FOREACH:%[0-9]+]] = function_ref @$sSTsE7forEachyyy7ElementQzKXEKF // CHECK-DAG: try_apply [[FOREACH]], inout Optional>, inout Optional>) -> ()>>({{%.*}}, [[SB:%[0-9]+]]) - // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY2:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] - // CHECK-DAG: [[ARGSARRAY2]] = begin_borrow [[ARGSARRAY3:%[0-9]+]] + // CHECK-DAG: [[SB]] = store_borrow [[ARGSARRAY3:%[0-9]+]] to [[ARGSARRAYADDR:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY3]] = copy_value [[ARGSARRAY4:%[0-9]+]] // CHECK-DAG: [[ARGSARRAY4]] = begin_borrow [[FINARR:%[0-9]+]] // CHECK-DAG: [[FINARRFUNC:%[0-9]+]] = function_ref @$ss27_finalizeUninitializedArrayySayxGABnlF diff --git a/test/SILOptimizer/moveonly_resilient_property_reader.swift b/test/SILOptimizer/moveonly_resilient_property_reader.swift new file mode 100644 index 0000000000000..46e89d8d5196a --- /dev/null +++ b/test/SILOptimizer/moveonly_resilient_property_reader.swift @@ -0,0 +1,15 @@ +// RUN: %target-swift-frontend -enable-experimental-feature MoveOnlyResilientTypes -enable-library-evolution -emit-sil %s | %FileCheck %s + +public struct ResilientMemberC {} +public struct ResilientMemberNC: ~Copyable {} + +public struct Resilient: ~Copyable { + // CHECK-LABEL: sil @${{.*}}9pubPropNC{{.*}}vr : + // CHECK: [[BASE:%.*]] = load {{.*}} : $*Resilient + // CHECK: [[PROJ_BUF:%.*]] = alloc_stack $ResilientMemberNC + // CHECK: [[PROJ:%.*]] = struct_extract [[BASE]] + // CHECK: store [[PROJ]] to [[PROJ_BUF]] + // CHECK: yield [[PROJ_BUF]] + public var pubPropNC: ResilientMemberNC = ResilientMemberNC() +} + From be796bb01bddebd5bdfd3dd1e45b9fdae1e35653 Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Sat, 14 Oct 2023 10:00:21 -0700 Subject: [PATCH 2/7] ClosureLifetimeFixup: Fix parameter indexing issue. When deciding whether to clean up copies of noncopyable captures, I got the parameter indexing wrong when the closure has non-capture arguments. Fixing this allows noncopyable captures to work in more general circumstances. --- .../Mandatory/ClosureLifetimeFixup.cpp | 14 +++++++++----- .../moveonly_nonescaping_closures.swift | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp b/lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp index 3d06f4a46c88c..8af40201a3405 100644 --- a/lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp +++ b/lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp @@ -725,6 +725,9 @@ static SILValue tryRewriteToPartialApplyStack( SmallSetVector borrowedOriginals; + unsigned appliedArgStartIdx = + newPA->getOrigCalleeType()->getNumParameters() - newPA->getNumArguments(); + for (unsigned i : indices(newPA->getArgumentOperands())) { auto &arg = newPA->getArgumentOperands()[i]; SILValue copy = arg.get(); @@ -747,11 +750,12 @@ static SILValue tryRewriteToPartialApplyStack( } // Is the capture a borrow? - auto paramIndex = newPA - ->getArgumentIndexForOperandIndex(i + newPA->getArgumentOperandNumber()) - .value(); - if (!newPA->getOrigCalleeType()->getParameters()[paramIndex] - .isIndirectInGuaranteed()) { + + auto paramIndex = i + appliedArgStartIdx; + auto param = newPA->getOrigCalleeType()->getParameters()[paramIndex]; + LLVM_DEBUG(param.print(llvm::dbgs()); + llvm::dbgs() << '\n'); + if (!param.isIndirectInGuaranteed()) { LLVM_DEBUG(llvm::dbgs() << "-- not an in_guaranteed parameter\n"; newPA->getOrigCalleeType()->getParameters()[paramIndex] .print(llvm::dbgs()); diff --git a/test/SILOptimizer/moveonly_nonescaping_closures.swift b/test/SILOptimizer/moveonly_nonescaping_closures.swift index c2c1bec70ee7e..bff4e78f5b9e9 100644 --- a/test/SILOptimizer/moveonly_nonescaping_closures.swift +++ b/test/SILOptimizer/moveonly_nonescaping_closures.swift @@ -17,6 +17,8 @@ struct M { #else private var x: Int = 0 #endif + + borrowing func borrow() {} } func borrow(_: borrowing M) {} @@ -156,6 +158,18 @@ func p(x: inout M) { clodger({ consume(x); x = M() }) } +func takesClosureWithArg(_: (Int) -> ()) {} + +func invokesWithClosureWithArg() { + let m = M() + + takesClosureWithArg { _ in + m.borrow() + } + + m.borrow() +} + // need test cases for: // - capturing local let // - capturing local var From c33f7b10e1f841a080b828a7b5401c90891d32ef Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Tue, 17 Oct 2023 13:42:04 -0700 Subject: [PATCH 3/7] SILGen: Treat projections from non-implicitly-copyable parameters as borrow bases. --- lib/SILGen/SILGenLValue.cpp | 38 ++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/SILGen/SILGenLValue.cpp b/lib/SILGen/SILGenLValue.cpp index ed565d12cd6b5..c1c6186787e08 100644 --- a/lib/SILGen/SILGenLValue.cpp +++ b/lib/SILGen/SILGenLValue.cpp @@ -2935,10 +2935,7 @@ class LLVM_LIBRARY_VISIBILITY SILGenBorrowedBaseVisitor SILGenBorrowedBaseVisitor(SILGenLValue &SGL, SILGenFunction &SGF) : SGL(SGL), SGF(SGF) {} - /// Returns the subexpr static bool isNonCopyableBaseBorrow(SILGenFunction &SGF, Expr *e) { - if (auto *le = dyn_cast(e)) - return le->getType()->isPureMoveOnly(); if (auto *m = dyn_cast(e)) { // If our m is a pure noncopyable type or our base is, we need to perform // a noncopyable base borrow. @@ -2949,8 +2946,39 @@ class LLVM_LIBRARY_VISIBILITY SILGenBorrowedBaseVisitor return m->getType()->isPureMoveOnly() || m->getBase()->getType()->isPureMoveOnly(); } - if (auto *d = dyn_cast(e)) - return e->getType()->isPureMoveOnly(); + + if (auto *le = dyn_cast(e)) { + // Noncopyable type is obviously noncopyable. + if (le->getType()->isPureMoveOnly()) { + return true; + } + // Otherwise, check if the thing we're loading from is a noncopyable + // param decl. + e = le->getSubExpr(); + // fall through... + } + + if (auto *de = dyn_cast(e)) { + // Noncopyable type is obviously noncopyable. + if (de->getType()->isPureMoveOnly()) { + return true; + } + // If the decl ref refers to a parameter with an explicit ownership + // modifier, it is not implicitly copyable. + if (auto pd = dyn_cast(de->getDecl())) { + switch (pd->getSpecifier()) { + case ParamSpecifier::Borrowing: + case ParamSpecifier::Consuming: + return true; + case ParamSpecifier::Default: + case ParamSpecifier::InOut: + case ParamSpecifier::LegacyShared: + case ParamSpecifier::LegacyOwned: + return false; + } + llvm_unreachable("unhandled switch case"); + } + } return false; } From 1030b62b3a4ff4b685b69a3b42c71b8f74a28efb Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Tue, 17 Oct 2023 14:32:26 -0700 Subject: [PATCH 4/7] Remove allow list for MarkUnresolvedNonCopyableValueInst markers. If SILGen emitted a marker, that ought to be indication enough that move only checking is necessary. --- .../Mandatory/MoveOnlyObjectCheckerUtils.cpp | 220 +----------------- 1 file changed, 1 insertion(+), 219 deletions(-) diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp index 0bf6336699952..721fefd6130cd 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyObjectCheckerUtils.cpp @@ -82,225 +82,7 @@ bool swift::siloptimizer:: if (!mmci || !mmci->hasMoveCheckerKind() || !mmci->getType().isObject()) continue; - // Handle guaranteed/owned move arguments and values. - // - // We are pattern matching against these patterns: - // - // bb0(%0 : @guaranteed $T): - // %1 = copy_value %0 - // %2 = mark_unresolved_non_copyable_value [no_consume_or_assign] %1 - // bb0(%0 : @owned $T): - // %1 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 - // - // This is forming a let or an argument. - // bb0: - // %1 = move_value [lexical] %0 - // %2 = mark_unresolved_non_copyable_value [consumable_and_assignable] - // %1 - // - // This occurs when SILGen materializes a temporary move only value? - // bb0: - // %1 = begin_borrow [lexical] %0 - // %2 = copy_value %1 - // %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 - if (mmci->getOperand()->getType().isMoveOnly() && - !mmci->getOperand()->getType().isMoveOnlyWrapped()) { - if (auto *cvi = dyn_cast(mmci->getOperand())) { - if (auto *arg = dyn_cast(cvi->getOperand())) { - if (arg->getOwnershipKind() == OwnershipKind::Guaranteed) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - - // In the case we have a resilient argument, we may have the following - // pattern: - // - // bb0(%0 : $*Type): // in_guaranteed - // %1 = load_borrow %0 - // %2 = copy_value - // %3 = mark_unresolved_non_copyable_value [no_copy_or_assign] - if (auto *lbi = dyn_cast(cvi->getOperand())) { - if (auto *arg = dyn_cast(lbi->getOperand())) { - if (arg->getKnownParameterInfo().isIndirectInGuaranteed()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - - if (auto *bbi = dyn_cast(cvi->getOperand())) { - if (bbi->isLexical()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - - // Any time we have a move_value, we can process it. - if (auto *mvi = dyn_cast(mmci->getOperand())) { - moveIntroducersToProcess.insert(mmci); - continue; - } - - if (auto *arg = dyn_cast(mmci->getOperand())) { - if (arg->getOwnershipKind() == OwnershipKind::Owned) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - - // Handle guaranteed arguments. - // - // We are pattern matching this pattern: - // - // bb0(%0 : @guaranteed $T): - // %1 = copyable_to_moveonlywrapper [guaranteed] %0 - // %2 = copy_value %1 - // %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 - // - // NOTE: Unlike with owned arguments, we do not need to insert a - // begin_borrow lexical since the lexical value comes from the guaranteed - // argument itself. - // - // NOTE: When we are done checking, we will eliminate the copy_value, - // mark_unresolved_non_copyable_value inst to leave the IR in a guaranteed - // state. - if (auto *cvi = dyn_cast(mmci->getOperand())) { - if (auto *cvt = dyn_cast( - cvi->getOperand())) { - if (auto *arg = dyn_cast(cvt->getOperand())) { - if (arg->isNoImplicitCopy() && - arg->getOwnershipKind() == OwnershipKind::Guaranteed) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - } - - // Handle trivial arguments and values. - // - // In the instruction stream this looks like: - // - // bb0(%0 : $Trivial): - // %1 = copyable_to_moveonlywrapper [owned] %0 - // %2 = move_value [lexical] %1 - // %3 = mark_unresolved_non_copyable_value [consumable_and_assignable] %2 - // - // *OR* - // - // bb0(%0 : $Trivial): - // %1 = copyable_to_moveonlywrapper [owned] %0 - // %2 = move_value [lexical] %1 - // %3 = mark_unresolved_non_copyable_value [no_consume_or_assign] %2 - // - // We are relying on a structural SIL requirement that %0 has only one - // use, %1. This is validated by the SIL verifier. In this case, we need - // the move_value [lexical] to ensure that we get a lexical scope for the - // non-trivial value. - if (auto *mvi = dyn_cast(mmci->getOperand())) { - if (mvi->isLexical()) { - if (auto *cvt = dyn_cast( - mvi->getOperand())) { - if (cvt->getOperand()->getType().isTrivial(*fn)) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - } - - // Handle owned arguments. - // - // We are pattern matching this: - // - // bb0(%0 : @owned $T): - // %1 = copyable_to_moveonlywrapper [owned] %0 - // %2 = move_value [lexical] %1 - // %3 = mark_unresolved_non_copyable_value - // [consumable_and_assignable_owned] %2 - if (auto *mvi = dyn_cast(mmci->getOperand())) { - if (mvi->isLexical()) { - if (auto *cvt = dyn_cast( - mvi->getOperand())) { - if (auto *arg = dyn_cast(cvt->getOperand())) { - if (arg->isNoImplicitCopy()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - } - } - - // Handle non-trivial values. - // - // We are looking for the following pattern: - // - // %1 = begin_borrow [lexical] %0 - // %2 = copy_value %1 - // %3 = copyable_to_moveonlywrapper [owned] %2 - // %4 = mark_unresolved_non_copyable_value [consumable_and_assignable] - // - // Or for a move only type, we look for a move_value [lexical]. - if (auto *mvi = dyn_cast( - mmci->getOperand())) { - if (auto *cvi = dyn_cast(mvi->getOperand())) { - if (auto *bbi = dyn_cast(cvi->getOperand())) { - if (bbi->isLexical()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - } - - // Handle trivial values. - // - // We pattern match: - // - // %1 = copyable_to_moveonlywrapper [owned] %0 - // %2 = move_value [lexical] %1 - // %3 = mark_unresolved_non_copyable_value [consumable_and_assignable] %2 - if (auto *cvi = dyn_cast(mmci->getOperand())) { - if (auto *bbi = dyn_cast(cvi->getOperand())) { - if (bbi->isLexical()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - - // Handle guaranteed parameters of a resilient type used by a resilient - // function inside the module in which the resilient type is defined. - if (auto *cvi = dyn_cast(mmci->getOperand())) { - if (auto *cmi = dyn_cast(cvi->getOperand())) { - if (auto *lbi = dyn_cast(cmi->getOperand())) { - if (auto *arg = dyn_cast(lbi->getOperand())) { - if (arg->getKnownParameterInfo().isIndirectInGuaranteed()) { - moveIntroducersToProcess.insert(mmci); - continue; - } - } - } - } - } - - // If we see a mark_unresolved_non_copyable_value that is marked no - // implicit copy that we don't understand, emit a diagnostic to fail the - // compilation. This ensures that if someone marks something no implicit - // copy and we fail to check it, we fail the compilation. - // - // We then RAUW the mark_unresolved_non_copyable_value once we have - // emitted the error since later passes expect that - // mark_unresolved_non_copyable_value has been eliminated by us. Since we - // are failing already, this is ok to do. - emitter.emitCheckerDoesntUnderstandDiagnostic(mmci); - mmci->replaceAllUsesWith(mmci->getOperand()); - mmci->eraseFromParent(); - localChanged = true; + moveIntroducersToProcess.insert(mmci); } } return localChanged; From 28a69692f2c0d61c55c53101380f8caca2a36f32 Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Wed, 18 Oct 2023 18:03:25 -0700 Subject: [PATCH 5/7] SILGen: Avoid redundant materialization when reabstracting a property base from loadable to in-memory. The code here was materializing the value in this case, but then re-loading it, which is unnecessary since call emission will materialize the value to match the callee's calling convention already. It does look like a copy into formal evaluation scope is necessary to get the correct lifetimes in some circumstances. The move-only checker doesn't like any additional copies at all because it thinks they're consumes, so only borrow in the case where the value is move-only. --- lib/SILGen/SILGenApply.cpp | 23 ++++---- test/SILGen/cf_members.swift | 4 +- ...ly_loadable_to_address_reabstraction.swift | 53 +++++++++++++++++++ 3 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index 4d27f2e227a71..8c84e98bc86af 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -6517,9 +6517,9 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() { if (selfParam.isConsumed() && !base.hasCleanup()) { base = base.copyUnmanaged(SGF, loc); } - - // If the parameter is indirect, we need to drop the value into - // temporary memory. + // If the parameter is indirect, we'll need to drop the value into + // temporary memory. Make a copy scoped to the current formal access that + // we can materialize later. if (SGF.silConv.isSILIndirect(selfParam)) { // It's a really bad idea to materialize when we're about to // pass a value to an inout argument, because it's a really easy @@ -6529,14 +6529,17 @@ ArgumentSource AccessorBaseArgPreparer::prepareAccessorObjectBaseArg() { // itself). assert(!selfParam.isIndirectMutating() && "passing unmaterialized r-value as inout argument"); - - base = base.formallyMaterialize(SGF, loc); - auto shouldTake = IsTake_t(base.hasCleanup()); - base = SGF.emitFormalAccessLoad(loc, base.forward(SGF), - SGF.getTypeLowering(baseLoweredType), - SGFContext(), shouldTake); + + // Avoid copying the base if it's move-only. It should be OK to do in this + // case since the move-only base will be accessed in a formal access scope + // as well. This isn't always true for copyable bases so be less aggressive + // in that case. + if (base.getType().isMoveOnly()) { + base = base.formalAccessBorrow(SGF, loc); + } else { + base = base.formalAccessCopy(SGF, loc); + } } - return ArgumentSource(loc, RValue(SGF, loc, baseFormalType, base)); } diff --git a/test/SILGen/cf_members.swift b/test/SILGen/cf_members.swift index 07af6f29d0ade..719d7bbea1420 100644 --- a/test/SILGen/cf_members.swift +++ b/test/SILGen/cf_members.swift @@ -124,10 +124,8 @@ public func foo(_ x: Double) { // CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[Z]] : $*Struct1 // CHECK: [[ZVAL:%.*]] = load [trivial] [[READ]] // CHECK: store [[ZVAL]] to [trivial] [[ZTMP:%.*]] : - // CHECK: [[ZVAL_2:%.*]] = load [trivial] [[ZTMP]] - // CHECK: store [[ZVAL_2]] to [trivial] [[ZTMP_2:%.*]] : // CHECK: [[GET:%.*]] = function_ref @IAMStruct1GetRadius : $@convention(c) (@in Struct1) -> Double - // CHECK: apply [[GET]]([[ZTMP_2]]) + // CHECK: apply [[GET]]([[ZTMP]]) _ = z.radius // CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[Z]] : $*Struct1 // CHECK: [[ZVAL:%.*]] = load [trivial] [[READ]] diff --git a/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift b/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift new file mode 100644 index 0000000000000..946cce8577852 --- /dev/null +++ b/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift @@ -0,0 +1,53 @@ +// RUN: %target-swift-frontend -emit-sil -verify -enable-library-evolution -enable-experimental-feature MoveOnlyResilientTypes %s + +// Verify that call sequences that require reabstracting a noncopyable value +// from a loadable representation to an in-memory one are properly allowed by +// the move-only checker. + +public struct Foo: ~Copyable { + private var x: String = "" + + public var isEmpty: Bool { + @_silgen_name("get_isEmpty") get + } +} + +public struct Bar: ~Copyable { + public var x = Foo() + + public consuming func foo() { + // `Foo` is internally a loadable type, but it's public with library + // evolution enabled, so public members like `isEmpty` take their + // argument indirectly. + if x.isEmpty {} + //else { bar(x) } + } +} + +@_silgen_name("bar") +func bar(_: consuming Foo) + +func foo(_ x: consuming Foo) { + // `[String]` is a loadable copyable type, but we're using the + // `consuming` modifier on `x` to suppress implicit copyability. + // `isEmpty` is a method from the Collection protocol, so it takes `self` + // generically and therefore indirectly. + if x.isEmpty {} + else { bar(x) } +} + +func copyableBar(_: consuming [String]) {} + +func copyableFoo(prefix: consuming [String]) { + if prefix.isEmpty { } + else { copyableBar(prefix) } +} + +struct CopyableFoo { + var prefix: [String] + + consuming func foo() { + if prefix.isEmpty { } + else { copyableBar(prefix) } + } +} From 447e00d02c1eeb08c7a0a711f6fa4e387279ce8b Mon Sep 17 00:00:00 2001 From: Andrew Trick Date: Fri, 13 Oct 2023 20:54:30 -0700 Subject: [PATCH 6/7] Fix DiagnoseLifetimeIssues handling of nonescaping arguments. Function call arguments were not being treated as liveness uses. Unblocks SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes. #69169 https://github.com/apple/swift/pull/69169 --- .../Mandatory/DiagnoseLifetimeIssues.cpp | 2 + .../SILOptimizer/diagnose_lifetime_issues.sil | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp b/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp index 91b8b8b711337..0f4c42e49371e 100644 --- a/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp +++ b/lib/SILOptimizer/Mandatory/DiagnoseLifetimeIssues.cpp @@ -189,6 +189,8 @@ visitUses(SILValue def, bool updateLivenessAndWeakStores, int callDepth) { // Try to get information from the called function. switch (getArgumentState(ai, use, callDepth)) { case DoesNotEscape: + if (updateLivenessAndWeakStores) + liveness->updateForUse(user, /*lifetimeEnding*/ false); break; case CanEscape: return CanEscape; diff --git a/test/SILOptimizer/diagnose_lifetime_issues.sil b/test/SILOptimizer/diagnose_lifetime_issues.sil index 8523b9d78353f..5a75d7112f4cd 100644 --- a/test/SILOptimizer/diagnose_lifetime_issues.sil +++ b/test/SILOptimizer/diagnose_lifetime_issues.sil @@ -27,6 +27,7 @@ class WeakCycle { weak var c: WeakCycle? } +sil [ossa] @$s24diagnose_lifetime_issues8DelegateCACycfC : $@convention(method) (@thick Delegate.Type) -> @owned Delegate sil [ossa] @$s24diagnose_lifetime_issues10MyDelegateCACycfC : $@convention(method) (@thick MyDelegate.Type) -> @owned MyDelegate sil [ossa] @$s24diagnose_lifetime_issues14strongDelegate1dyAA0D0C_tF : $@convention(method) (@guaranteed Delegate, @guaranteed Container) -> () @@ -119,3 +120,41 @@ bb0(%0 : $@thick WeakCycle.Type): %3 = apply %2(%1) : $@convention(method) (@owned WeakCycle) -> @owned WeakCycle return %3 : $WeakCycle } + +// Helper +sil private [ossa] @testBorrowInDefer$defer : $@convention(thin) (@guaranteed Delegate) -> () { +bb0(%0 : @closureCapture @guaranteed $Delegate): + debug_value %0 : $Delegate, let, name "delegate", argno 1 + fix_lifetime %0 : $Delegate + %8 = tuple () + return %8 : $() +} + +// Test no warning for a value kept alive within a call which does not escape its argument. +sil hidden [ossa] @testBorrowinDefer : $@convention(thin) (@guaranteed Container) -> () { +bb0(%0 : @guaranteed $Container): + debug_value %0 : $Container, let, name "container", argno 1 + %2 = metatype $@thick Delegate.Type + // function_ref Delegate.__allocating_init() + %3 = function_ref @$s24diagnose_lifetime_issues8DelegateCACycfC : $@convention(method) (@thick Delegate.Type) -> @owned Delegate + + // This is the owned allocation. + %4 = apply %3(%2) : $@convention(method) (@thick Delegate.Type) -> @owned Delegate + %6 = copy_value %4 : $Delegate + %7 = enum $Optional, #Optional.some!enumelt, %6 : $Delegate + %8 = ref_element_addr %0 : $Container, #Container.delegate + %9 = begin_access [modify] [dynamic] %8 : $*@sil_weak Optional + + // This is the weak assignment. + store_weak %7 to %9 : $*@sil_weak Optional + destroy_value %7 : $Optional + end_access %9 : $*@sil_weak Optional + + // This call keeps the parent alive + %15 = function_ref @testBorrowInDefer$defer : $@convention(thin) (@guaranteed Delegate) -> () // user: %16 + %16 = apply %15(%4) : $@convention(thin) (@guaranteed Delegate) -> () + + destroy_value %4 : $Delegate + %18 = tuple () + return %18 : $() +} From ca6b7a40f59058949fd0058b2649b295bdf78e04 Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Thu, 19 Oct 2023 11:46:08 -0700 Subject: [PATCH 7/7] Enable MoveOnlyResilientTypes feature. --- include/swift/Basic/Features.def | 2 +- lib/Sema/TypeCheckDeclPrimary.cpp | 12 ------ ...dule_source_deinit_library_evolution.swift | 4 +- test/ModuleInterface/discard_interface.swift | 4 +- .../moveonly_interface_flag.swift | 4 +- test/ModuleInterface/moveonly_user.swift | 4 +- test/SILGen/moveonly_library_evolution.swift | 4 +- ...hecker_diagnostics_library_evolution.swift | 2 +- ...nit_devirtualization_library_evolution.sil | 2 +- ...ly_loadable_to_address_reabstraction.swift | 2 +- .../moveonly_resilient_property_reader.swift | 2 +- test/Sema/discard_module.swift | 2 +- test/Sema/moveonly_resilient_type.swift | 37 ------------------- 13 files changed, 16 insertions(+), 65 deletions(-) delete mode 100644 test/Sema/moveonly_resilient_type.swift diff --git a/include/swift/Basic/Features.def b/include/swift/Basic/Features.def index 71f5ddf75bed0..77832b4d9556a 100644 --- a/include/swift/Basic/Features.def +++ b/include/swift/Basic/Features.def @@ -107,6 +107,7 @@ LANGUAGE_FEATURE( LANGUAGE_FEATURE(AttachedMacros, 389, "Attached macros", hasSwiftSwiftParser) LANGUAGE_FEATURE(ExtensionMacros, 402, "Extension macros", hasSwiftSwiftParser) LANGUAGE_FEATURE(MoveOnly, 390, "noncopyable types", true) +LANGUAGE_FEATURE(MoveOnlyResilientTypes, 390, "non-@frozen noncopyable types with library evolution", true) LANGUAGE_FEATURE(ParameterPacks, 393, "Value and type parameter packs", true) SUPPRESSIBLE_LANGUAGE_FEATURE(LexicalLifetimes, 0, "@_eagerMove/@_noEagerMove/@_lexicalLifetimes annotations", true) LANGUAGE_FEATURE(FreestandingMacros, 397, "freestanding declaration macros", true) @@ -140,7 +141,6 @@ EXPERIMENTAL_FEATURE(NoImplicitCopy, true) EXPERIMENTAL_FEATURE(OldOwnershipOperatorSpellings, true) EXPERIMENTAL_FEATURE(MoveOnlyEnumDeinits, true) EXPERIMENTAL_FEATURE(MoveOnlyTuples, true) -EXPERIMENTAL_FEATURE(MoveOnlyResilientTypes, true) EXPERIMENTAL_FEATURE(MoveOnlyPartialConsumption, true) EXPERIMENTAL_FEATURE(OneWayClosureParameters, false) diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index 3bd32503686dc..903aac7057eaf 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -2794,12 +2794,6 @@ class DeclChecker : public DeclVisitor { ED->getBaseIdentifier()); } } - - if (!ED->getASTContext().LangOpts.hasFeature( - Feature::MoveOnlyResilientTypes) && - ED->isResilient()) { - ED->diagnose(diag::noncopyable_types_cannot_be_resilient, ED); - } } } @@ -2840,12 +2834,6 @@ class DeclChecker : public DeclVisitor { diagnoseCopyableTypeContainingMoveOnlyType(SD); diagnoseIncompatibleProtocolsForMoveOnlyType(SD); - - if (!SD->getASTContext().LangOpts.hasFeature( - Feature::MoveOnlyResilientTypes) && - SD->isResilient() && SD->isMoveOnly()) { - SD->diagnose(diag::noncopyable_types_cannot_be_resilient, SD); - } } /// Check whether the given properties can be @NSManaged in this class. diff --git a/test/IRGen/moveonly_split_module_source_deinit_library_evolution.swift b/test/IRGen/moveonly_split_module_source_deinit_library_evolution.swift index d6cb6f413ba3f..45910f262f528 100644 --- a/test/IRGen/moveonly_split_module_source_deinit_library_evolution.swift +++ b/test/IRGen/moveonly_split_module_source_deinit_library_evolution.swift @@ -1,8 +1,8 @@ // RUN: %empty-directory(%t) -// RUN: %target-build-swift-dylib(%t/%target-library-name(MoveOnlySplit)) -enable-library-evolution %S/Inputs/moveonly_split_module_source_input.swift -emit-module -emit-module-path %t/MoveOnlySplit.swiftmodule -module-name MoveOnlySplit -DTEST_LIBRARY_EVOLUTION -enable-experimental-feature MoveOnlyResilientTypes +// RUN: %target-build-swift-dylib(%t/%target-library-name(MoveOnlySplit)) -enable-library-evolution %S/Inputs/moveonly_split_module_source_input.swift -emit-module -emit-module-path %t/MoveOnlySplit.swiftmodule -module-name MoveOnlySplit -DTEST_LIBRARY_EVOLUTION // RUN: %target-codesign %t/%target-library-name(MoveOnlySplit) -// RUN: %target-build-swift %s -lMoveOnlySplit -I %t -L %t -o %t/main %target-rpath(%t) -enable-experimental-feature MoveOnlyResilientTypes +// RUN: %target-build-swift %s -lMoveOnlySplit -I %t -L %t -o %t/main %target-rpath(%t) // RUN: %target-codesign %t/main // RUN: %target-run %t/main %t/%target-library-name(MoveOnlySplit) | %FileCheck -check-prefix=CHECK-LIBRARY-EVOLUTION %s diff --git a/test/ModuleInterface/discard_interface.swift b/test/ModuleInterface/discard_interface.swift index 867f075dfbb07..ece601339d7e0 100644 --- a/test/ModuleInterface/discard_interface.swift +++ b/test/ModuleInterface/discard_interface.swift @@ -1,6 +1,6 @@ // RUN: %empty-directory(%t) -// RUN: %target-swift-emit-module-interface(%t/Library.swiftinterface) %s -module-name Library -verify -enable-experimental-feature MoveOnlyResilientTypes -// RUN: %target-swift-typecheck-module-from-interface(%t/Library.swiftinterface) -I %t -enable-experimental-feature MoveOnlyResilientTypes +// RUN: %target-swift-emit-module-interface(%t/Library.swiftinterface) %s -module-name Library -verify +// RUN: %target-swift-typecheck-module-from-interface(%t/Library.swiftinterface) -I %t // RUN: %FileCheck %s < %t/Library.swiftinterface // This test makes sure that discard is emitted correctly in the interfaces. diff --git a/test/ModuleInterface/moveonly_interface_flag.swift b/test/ModuleInterface/moveonly_interface_flag.swift index 8d81052854a3e..1ee6aa4178375 100644 --- a/test/ModuleInterface/moveonly_interface_flag.swift +++ b/test/ModuleInterface/moveonly_interface_flag.swift @@ -1,6 +1,6 @@ // RUN: %empty-directory(%t) -// RUN: %target-swift-emit-module-interface(%t/Library.swiftinterface) %s -module-name Library -enable-experimental-feature MoveOnlyResilientTypes -// RUN: %target-swift-typecheck-module-from-interface(%t/Library.swiftinterface) -I %t -enable-experimental-feature MoveOnlyResilientTypes +// RUN: %target-swift-emit-module-interface(%t/Library.swiftinterface) %s -module-name Library +// RUN: %target-swift-typecheck-module-from-interface(%t/Library.swiftinterface) -I %t // RUN: %FileCheck %s < %t/Library.swiftinterface // this test makes sure that decls containing a move-only type are guarded by the $MoveOnly feature flag diff --git a/test/ModuleInterface/moveonly_user.swift b/test/ModuleInterface/moveonly_user.swift index 524d4a74194d3..5b60d7b87426b 100644 --- a/test/ModuleInterface/moveonly_user.swift +++ b/test/ModuleInterface/moveonly_user.swift @@ -5,8 +5,8 @@ // RUN: %target-swift-frontend -emit-sil -sil-verify-all -I %t %s > /dev/null // >> now again with library evolution; we expect the same result. -// RUN: %target-swift-frontend -DSYNTHESIZE_ACCESSORS -enable-library-evolution -enable-experimental-feature MoveOnlyResilientTypes -emit-module -o %t/Hello.swiftmodule %S/Inputs/moveonly_api.swift -// RUN: %target-swift-frontend -enable-experimental-feature MoveOnlyResilientTypes -emit-sil -sil-verify-all -I %t %s > /dev/null +// RUN: %target-swift-frontend -DSYNTHESIZE_ACCESSORS -enable-library-evolution -emit-module -o %t/Hello.swiftmodule %S/Inputs/moveonly_api.swift +// RUN: %target-swift-frontend -emit-sil -sil-verify-all -I %t %s > /dev/null // FIXME: ideally this would also try executing the program rather than just generating SIL diff --git a/test/SILGen/moveonly_library_evolution.swift b/test/SILGen/moveonly_library_evolution.swift index 2ba89329919c9..ad65d16abe081 100644 --- a/test/SILGen/moveonly_library_evolution.swift +++ b/test/SILGen/moveonly_library_evolution.swift @@ -1,5 +1,5 @@ -// RUN: %target-swift-emit-silgen -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyResilientTypes -enable-library-evolution %s | %FileCheck %s -// RUN: %target-swift-emit-sil -O -sil-verify-all -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-experimental-feature MoveOnlyResilientTypes -enable-library-evolution %s +// RUN: %target-swift-emit-silgen -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-library-evolution %s | %FileCheck %s +// RUN: %target-swift-emit-sil -O -sil-verify-all -enable-experimental-feature MoveOnlyPartialConsumption -enable-experimental-feature NoImplicitCopy -enable-library-evolution %s //////////////////////// // MARK: Declarations // diff --git a/test/SILOptimizer/moveonly_addresschecker_diagnostics_library_evolution.swift b/test/SILOptimizer/moveonly_addresschecker_diagnostics_library_evolution.swift index f222107a9965d..e5fa74eec7ce0 100644 --- a/test/SILOptimizer/moveonly_addresschecker_diagnostics_library_evolution.swift +++ b/test/SILOptimizer/moveonly_addresschecker_diagnostics_library_evolution.swift @@ -1,4 +1,4 @@ -// RUN: %target-swift-emit-sil -enable-experimental-feature NoImplicitCopy -sil-verify-all -verify -enable-library-evolution -enable-experimental-feature MoveOnlyResilientTypes %s +// RUN: %target-swift-emit-sil -enable-experimental-feature NoImplicitCopy -sil-verify-all -verify -enable-library-evolution %s // This test is used to validate that we properly handle library evolution code // until we can get all of the normal moveonly_addresschecker_diagnostics test diff --git a/test/SILOptimizer/moveonly_deinit_devirtualization_library_evolution.sil b/test/SILOptimizer/moveonly_deinit_devirtualization_library_evolution.sil index ded6546839a72..1dd98cbd32c8e 100644 --- a/test/SILOptimizer/moveonly_deinit_devirtualization_library_evolution.sil +++ b/test/SILOptimizer/moveonly_deinit_devirtualization_library_evolution.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -enable-library-evolution -module-name main -enable-sil-verify-all -sil-move-only-deinit-devirtualization -enable-experimental-feature MoveOnlyClasses -enable-experimental-feature MoveOnlyEnumDeinits -enable-experimental-feature MoveOnlyResilientTypes %s | %FileCheck %s +// RUN: %target-sil-opt -enable-library-evolution -module-name main -enable-sil-verify-all -sil-move-only-deinit-devirtualization -enable-experimental-feature MoveOnlyClasses -enable-experimental-feature MoveOnlyEnumDeinits %s | %FileCheck %s sil_stage raw diff --git a/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift b/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift index 946cce8577852..347c74ec39349 100644 --- a/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift +++ b/test/SILOptimizer/moveonly_loadable_to_address_reabstraction.swift @@ -1,4 +1,4 @@ -// RUN: %target-swift-frontend -emit-sil -verify -enable-library-evolution -enable-experimental-feature MoveOnlyResilientTypes %s +// RUN: %target-swift-frontend -emit-sil -verify -enable-library-evolution %s // Verify that call sequences that require reabstracting a noncopyable value // from a loadable representation to an in-memory one are properly allowed by diff --git a/test/SILOptimizer/moveonly_resilient_property_reader.swift b/test/SILOptimizer/moveonly_resilient_property_reader.swift index 46e89d8d5196a..ee48546784aa3 100644 --- a/test/SILOptimizer/moveonly_resilient_property_reader.swift +++ b/test/SILOptimizer/moveonly_resilient_property_reader.swift @@ -1,4 +1,4 @@ -// RUN: %target-swift-frontend -enable-experimental-feature MoveOnlyResilientTypes -enable-library-evolution -emit-sil %s | %FileCheck %s +// RUN: %target-swift-frontend -enable-library-evolution -emit-sil %s | %FileCheck %s public struct ResilientMemberC {} public struct ResilientMemberNC: ~Copyable {} diff --git a/test/Sema/discard_module.swift b/test/Sema/discard_module.swift index aae8852bbc9e0..03dec35baaf8e 100644 --- a/test/Sema/discard_module.swift +++ b/test/Sema/discard_module.swift @@ -5,7 +5,7 @@ // RUN: %target-typecheck-verify-swift -I %t // >> now again with library evolution; we expect the same result. -// RUN: %target-swift-frontend -enable-library-evolution -emit-module -o %t/SorryModule.swiftmodule %S/Inputs/discard_module_defining.swift %S/Inputs/discard_module_adjacent.swift -enable-experimental-feature MoveOnlyResilientTypes +// RUN: %target-swift-frontend -enable-library-evolution -emit-module -o %t/SorryModule.swiftmodule %S/Inputs/discard_module_defining.swift %S/Inputs/discard_module_adjacent.swift // RUN: %target-typecheck-verify-swift -I %t // "Sorry" is meaningless diff --git a/test/Sema/moveonly_resilient_type.swift b/test/Sema/moveonly_resilient_type.swift deleted file mode 100644 index 45772bc726b50..0000000000000 --- a/test/Sema/moveonly_resilient_type.swift +++ /dev/null @@ -1,37 +0,0 @@ -// RUN: %target-swift-frontend -typecheck %s -enable-library-evolution -verify - -public struct ResilientStruct : ~Copyable { // expected-error {{noncopyable struct 'ResilientStruct' must be @frozen in library evolution mode; non-@frozen public and @usableFromInline noncopyable types are not supported}} -} - -@frozen -public struct FrozenStruct : ~Copyable { - public init() {} -} - -@usableFromInline -struct UsableFromInlineStruct : ~Copyable { // expected-error {{noncopyable struct 'UsableFromInlineStruct' must be @frozen in library evolution mode; non-@frozen public and @usableFromInline noncopyable types are not supported}} -} - -public enum ResilientEnum : ~Copyable { // expected-error {{noncopyable enum 'ResilientEnum' must be @frozen in library evolution mode; non-@frozen public and @usableFromInline noncopyable types are not supported}} -} - -@frozen -public enum FrozenEnum : ~Copyable { -} - -@usableFromInline -enum UsableFromInlineEnum : ~Copyable { // expected-error {{noncopyable enum 'UsableFromInlineEnum' must be @frozen in library evolution mode; non-@frozen public and @usableFromInline noncopyable types are not supported}} -} - -public class C { - @usableFromInline - var x: FrozenStruct - - public init() {} - - @inlinable - convenience public init(delegating: ()) { - self.init() - x = FrozenStruct() - } -}