diff --git a/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp b/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp index f3b8cf88144c6..3a53c9e498067 100644 --- a/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp +++ b/lib/SIL/Utils/FieldSensitivePrunedLiveness.cpp @@ -1137,9 +1137,18 @@ void FieldSensitivePrunedLiveRange::computeBoundary( for (SILBasicBlock *succBB : block->getSuccessors()) { if (FieldSensitivePrunedLiveBlocks::isDead( getBlockLiveness(succBB, index))) { - PRUNED_LIVENESS_LOG(llvm::dbgs() << "Marking succBB as boundary edge: bb" - << succBB->getDebugID() << '\n'); - boundary.getBoundaryEdgeBits(succBB).set(index); + // If the basic block ends in unreachable, don't consider it a + // boundary. + // TODO: Should also do this if the block's successors all always + // end in unreachable too. + if (isa(succBB->getTerminator())) { + PRUNED_LIVENESS_LOG(llvm::dbgs() << "succBB ends in unreachable, skipping as boundary edge: bb" + << succBB->getDebugID() << '\n'); + } else { + PRUNED_LIVENESS_LOG(llvm::dbgs() << "Marking succBB as boundary edge: bb" + << succBB->getDebugID() << '\n'); + boundary.getBoundaryEdgeBits(succBB).set(index); + } } } asImpl().findBoundariesInBlock(block, index, /*isLiveOut*/ true, diff --git a/lib/SILGen/SILGenApply.cpp b/lib/SILGen/SILGenApply.cpp index 17473d1c6f104..8278930296a94 100644 --- a/lib/SILGen/SILGenApply.cpp +++ b/lib/SILGen/SILGenApply.cpp @@ -3223,6 +3223,14 @@ static StorageRefResult findStorageReferenceExprForBorrow(Expr *e) { Expr *SILGenFunction::findStorageReferenceExprForMoveOnly(Expr *argExpr, StorageReferenceOperationKind kind) { + ForceValueExpr *forceUnwrap = nullptr; + // Check for a force unwrap. This might show up inside or outside of the + // load. + if (auto *fu = dyn_cast(argExpr)) { + forceUnwrap = fu; + argExpr = fu->getSubExpr(); + } + // If there's a load around the outer part of this arg expr, look past it. bool sawLoad = false; if (auto *li = dyn_cast(argExpr)) { @@ -3230,34 +3238,39 @@ Expr *SILGenFunction::findStorageReferenceExprForMoveOnly(Expr *argExpr, sawLoad = true; } + // Check again for a force unwrap before the load. + if (auto *fu = dyn_cast(argExpr)) { + forceUnwrap = fu; + argExpr = fu->getSubExpr(); + } + // If we're consuming instead, then the load _must_ have been there. if (kind == StorageReferenceOperationKind::Consume && !sawLoad) return nullptr; - // If we did not see a load and our argExpr is a - // declref_expr, return nullptr. We have an object not something that will be - // in memory. This can happen with classes or with values captured by a - // closure. - // - // NOTE: If we see a member_ref_expr from a decl_ref_expr, we still process it - // since the declref_expr could be from a class. + // TODO: This section should be removed eventually. Decl refs should not be + // handled different from other storage. Removing it breaks some things + // currently. if (!sawLoad) { if (auto *declRef = dyn_cast(argExpr)) { assert(!declRef->getType()->is() && "Shouldn't ever have an lvalue type here!"); - - // Proceed if the storage references a global or static let. - // TODO: We should treat any storage reference as a borrow, it seems, but - // that currently disrupts what the move checker expects. It would also - // be valuable to borrow copyable global lets, but this is a targeted - // fix to allow noncopyable globals to work properly. - bool isGlobal = false; - if (auto vd = dyn_cast(declRef->getDecl())) { - isGlobal = vd->isGlobalStorage(); - } - - if (!isGlobal) { - return nullptr; + + // Proceed if the storage reference is a force unwrap. + if (!forceUnwrap) { + // Proceed if the storage references a global or static let. + // TODO: We should treat any storage reference as a borrow, it seems, but + // that currently disrupts what the move checker expects. It would also + // be valuable to borrow copyable global lets, but this is a targeted + // fix to allow noncopyable globals to work properly. + bool isGlobal = false; + if (auto vd = dyn_cast(declRef->getDecl())) { + isGlobal = vd->isGlobalStorage(); + } + + if (!isGlobal) { + return nullptr; + } } } } @@ -3302,6 +3315,10 @@ Expr *SILGenFunction::findStorageReferenceExprForMoveOnly(Expr *argExpr, } if (!isMoveOnly) return nullptr; + + if (forceUnwrap) { + return forceUnwrap; + } return result.getTransitiveRoot(); } diff --git a/lib/SILGen/SILGenConvert.cpp b/lib/SILGen/SILGenConvert.cpp index f8d0190e6935a..73a2072372d32 100644 --- a/lib/SILGen/SILGenConvert.cpp +++ b/lib/SILGen/SILGenConvert.cpp @@ -199,9 +199,8 @@ SILGenFunction::emitPreconditionOptionalHasValue(SILLocation loc, auto someDecl = getASTContext().getOptionalSomeDecl(); auto noneDecl = getASTContext().getOptionalNoneDecl(); - // If we have an object, make sure the object is at +1. All switch_enum of - // objects is done at +1. bool isAddress = optional.getType().isAddress(); + bool isBorrow = !optional.isPlusOneOrTrivial(*this); SwitchEnumInst *switchEnum = nullptr; if (isAddress) { // We forward in the creation routine for @@ -209,6 +208,12 @@ SILGenFunction::emitPreconditionOptionalHasValue(SILLocation loc, B.createSwitchEnumAddr(loc, optional.getValue(), /*defaultDest*/ nullptr, {{someDecl, contBB}, {noneDecl, failBB}}); + } else if (isBorrow) { + hadCleanup = false; + hadLValue = false; + switchEnum = B.createSwitchEnum(loc, optional.getValue(), + /*defaultDest*/ nullptr, + {{someDecl, contBB}, {noneDecl, failBB}}); } else { optional = optional.ensurePlusOne(*this, loc); hadCleanup = true; diff --git a/lib/SILGen/SILGenLValue.cpp b/lib/SILGen/SILGenLValue.cpp index 20921e3291dbf..124733d1a6399 100644 --- a/lib/SILGen/SILGenLValue.cpp +++ b/lib/SILGen/SILGenLValue.cpp @@ -948,6 +948,9 @@ namespace { ManagedValue base) && override { // Assert that the optional value is present and return the projected out // payload. + if (isConsumeAccess(getTypeData().getAccessKind())) { + base = base.ensurePlusOne(SGF, loc); + } return SGF.emitPreconditionOptionalHasValue(loc, base, isImplicitUnwrap); } @@ -4355,6 +4358,16 @@ getOptionalObjectTypeData(SILGenFunction &SGF, SGFAccessKind accessKind, LValue SILGenLValue::visitForceValueExpr(ForceValueExpr *e, SGFAccessKind accessKind, LValueOptions options) { + // Since Sema doesn't reason about borrows, a borrowed force expr + // might end up type checked with the load inside of the force. + auto subExpr = e->getSubExpr(); + if (auto load = dyn_cast(subExpr)) { + assert((isBorrowAccess(accessKind) || isConsumeAccess(accessKind)) + && "should only see a (force_value (load)) lvalue as part of a " + "borrow or consume"); + subExpr = load->getSubExpr(); + } + // Like BindOptional, this is a read even if we only write to the result. // (But it's unnecessary to use a force this way!) LValue lv = visitRec(e->getSubExpr(), diff --git a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp index 733a391dee6ee..4523d0923438d 100644 --- a/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp +++ b/lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp @@ -1713,9 +1713,6 @@ struct CopiedLoadBorrowEliminationVisitor useWorklist.push_back(use); } - // If we have a switch_enum, we always need to convert it to a load - // [copy] since we need to destructure through it. - shouldConvertToLoadCopy |= isa(nextUse->getUser()); continue; } case OperandOwnership::Borrow: diff --git a/test/SILGen/vtable_thunks.swift b/test/SILGen/vtable_thunks.swift index 4260505f84585..2909bf3e4f949 100644 --- a/test/SILGen/vtable_thunks.swift +++ b/test/SILGen/vtable_thunks.swift @@ -145,18 +145,16 @@ class H: G { // CHECK-LABEL: sil private [thunk] [ossa] @$s13vtable_thunks1DC3iuo{{[_0-9a-zA-Z]*}}FTV // CHECK: bb0([[X:%.*]] : @guaranteed $B, [[Y:%.*]] : @guaranteed $Optional, [[Z:%.*]] : @guaranteed $B, [[W:%.*]] : @guaranteed $D): // CHECK: [[WRAP_X:%.*]] = enum $Optional, #Optional.some!enumelt, [[X]] : $B -// CHECK: [[Y_COPY:%.*]] = copy_value [[Y]] -// CHECK: switch_enum [[Y_COPY]] : $Optional, case #Optional.some!enumelt: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]] +// CHECK: switch_enum [[Y]] : $Optional, case #Optional.some!enumelt: [[SOME_BB:bb[0-9]+]], case #Optional.none!enumelt: [[NONE_BB:bb[0-9]+]] // CHECK: [[NONE_BB]]: // CHECK: [[DIAGNOSE_UNREACHABLE_FUNC:%.*]] = function_ref @$ss30_diagnoseUnexpectedNilOptional{{.*}} // CHECK: apply [[DIAGNOSE_UNREACHABLE_FUNC]] // CHECK: unreachable -// CHECK: [[SOME_BB]]([[UNWRAP_Y:%.*]] : @owned $B): -// CHECK: [[BORROWED_UNWRAP_Y:%.*]] = begin_borrow [[UNWRAP_Y]] +// CHECK: [[SOME_BB]]([[UNWRAP_Y:%.*]] : @guaranteed $B): // CHECK: [[THUNK_FUNC:%.*]] = function_ref @$s13vtable_thunks1DC3iuo{{.*}} -// CHECK: [[RES:%.*]] = apply [[THUNK_FUNC]]([[WRAP_X]], [[BORROWED_UNWRAP_Y]], [[Z]], [[W]]) +// CHECK: [[RES:%.*]] = apply [[THUNK_FUNC]]([[WRAP_X]], [[UNWRAP_Y]], [[Z]], [[W]]) // CHECK: [[WRAP_RES:%.*]] = enum $Optional, {{.*}} [[RES]] // CHECK: return [[WRAP_RES]] diff --git a/test/SILOptimizer/discard_checking.swift b/test/SILOptimizer/discard_checking.swift index f374bf91aca95..d96b01ea38da3 100644 --- a/test/SILOptimizer/discard_checking.swift +++ b/test/SILOptimizer/discard_checking.swift @@ -16,7 +16,7 @@ func globalThrowingFn() throws {} struct Basics: ~Copyable { consuming func test1(_ b: Bool) { guard b else { - fatalError("bah!") // expected-error {{must consume 'self' before exiting method that discards self}} + return // expected-error {{must consume 'self' before exiting method that discards self}} } discard self // expected-note {{discarded self here}} } @@ -24,7 +24,7 @@ struct Basics: ~Copyable { consuming func test1_fixed(_ b: Bool) { guard b else { _ = consume self - fatalError("bah!") + return } discard self } @@ -33,7 +33,7 @@ struct Basics: ~Copyable { repeat { switch c { case .red: - fatalError("bah!") + return case .blue: throw E.someError case .green: @@ -49,7 +49,7 @@ struct Basics: ~Copyable { switch c { case .red: discard self - fatalError("bah!") + return case .blue: discard self throw E.someError @@ -145,7 +145,7 @@ struct Basics: ~Copyable { if case .red = c { discard self // expected-note {{discarded self here}} } - fatalError("oh no") // expected-error {{must consume 'self' before exiting method that discards self}} + return // expected-error {{must consume 'self' before exiting method that discards self}} } consuming func test7_fixed(_ c: Color) throws { @@ -154,7 +154,7 @@ struct Basics: ~Copyable { return } _ = consume self - fatalError("oh no") + return } consuming func test8(_ c: Color) throws { @@ -162,9 +162,9 @@ struct Basics: ~Copyable { discard self // expected-note {{discarded self here}} } if case .blue = c { - fatalError("hi") // expected-error {{must consume 'self' before exiting method that discards self}} + return } - } + } // expected-error {{must consume 'self' before exiting method that discards self}} consuming func test8_stillMissingAConsume1(_ c: Color) throws { if case .red = c { @@ -173,7 +173,7 @@ struct Basics: ~Copyable { } if case .blue = c { _ = consume self - fatalError("hi") + return } } // expected-error {{must consume 'self' before exiting method that discards self}} @@ -183,7 +183,7 @@ struct Basics: ~Copyable { return } if case .blue = c { - fatalError("hi") // expected-error {{must consume 'self' before exiting method that discards self}} + return // expected-error {{must consume 'self' before exiting method that discards self}} } _ = consume self } @@ -195,7 +195,7 @@ struct Basics: ~Copyable { } if case .blue = c { _ = consume self - fatalError("hi") + return } _ = consume self } @@ -407,7 +407,7 @@ struct Basics: ~Copyable { case 2: return // expected-error {{must consume 'self' before exiting method that discards self}} case 3: - fatalError("no") // expected-error {{must consume 'self' before exiting method that discards self}} + return // expected-error {{must consume 'self' before exiting method that discards self}} case 4: globalConsumingFn(self) default: @@ -568,7 +568,7 @@ struct Money: ~Copyable { consuming func spend(_ charge: Int) throws -> Money { guard charge > 0 else { - fatalError("can't charge a negative amount!") // expected-error {{must consume 'self' before exiting method that discards self}} + return Money(balance: balance) // expected-error {{must consume 'self' before exiting method that discards self}} } if balance < charge { diff --git a/test/SILOptimizer/moveonly_optional_force_unwrap.swift b/test/SILOptimizer/moveonly_optional_force_unwrap.swift new file mode 100644 index 0000000000000..bb7f77bc5b5ec --- /dev/null +++ b/test/SILOptimizer/moveonly_optional_force_unwrap.swift @@ -0,0 +1,152 @@ +// RUN: %target-swift-frontend -enable-experimental-feature NoncopyableGenerics -emit-sil -verify %s + +struct NC: ~Copyable { + borrowing func borrow() {} + mutating func mutate() {} + consuming func consume() {} +} + +func borrow(_: borrowing NC) {} +func consume(_: consuming NC) {} +func mutate(_: inout NC) {} + +func unwrapBorrow_Borrow(x: borrowing NC?) { + x!.borrow() + borrow(x!) + + x!.borrow() + borrow(x!) +} + +func unwrapConsume_Borrow(x: borrowing NC?) { // expected-error{{cannot be consumed}} + x!.consume() // expected-note{{consumed here}} + consume(x!) // expected-note{{consumed here}} + + x!.consume() // expected-note{{consumed here}} + consume(x!) // expected-note{{consumed here}} +} + +func unwrapBorrowMutateConsume_Consume(x: consuming NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) +} + +func unwrapBorrowMutateConsume2_Consume(x: consuming NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + x!.consume() +} + +func unwrapBorrowMutateConsumeBorrow_Consume(x: consuming NC?) { // expected-error {{used after consume}} + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) // expected-note{{consumed here}} + + x!.borrow() // expected-note{{used here}} + borrow(x!) +} + +func unwrapBorrowMutateConsumeMutate_Consume(x: consuming NC?) { // expected-error {{used after consume}} + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) // expected-note{{consumed here}} + + x!.mutate() // expected-note{{used here}} + mutate(&x!) +} + +func unwrapBorrowMutateConsumeInitBorrow_Consume(x: consuming NC?, y: consuming NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) + + x = y + + x!.borrow() + borrow(x!) +} + +func unwrapBorrowMutateConsumeInitMutate_Consume(x: consuming NC?, y: consuming NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) + + x = y + + x!.mutate() + mutate(&x!) +} + +func unwrapBorrowMutateConsumeInitBorrowMutateConsume_Consume(x: consuming NC?, y: consuming NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + consume(x!) + + x = y + + x!.mutate() + x!.borrow() + mutate(&x!) + borrow(x!) + + consume(x!) +} + +func unwrapBorrowMutate_Mutate(x: inout NC?) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) +} + +func unwrapBorrowMutateConsume_Mutate(x: inout NC?) { // expected-error {{missing reinitialization}} + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + x!.consume() // expected-note {{consumed here}} +} + +func unwrapBorrowMutateConsumeInit_Mutate(x: inout NC?, y: consuming NC) { + x!.borrow() + x!.mutate() + + borrow(x!) + mutate(&x!) + + x!.consume() // expected-note{{consumed here}} + + x! = y // expected-error{{cannot partially reinitialize}} +}