Skip to content

[move-only] Teach the object/address move operator checker to not check move only types. #61964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions lib/SILGen/SILGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "swift/AST/Types.h"
#include "swift/Basic/SourceManager.h"
#include "swift/Basic/type_traits.h"
#include "swift/SIL/Consumption.h"
#include "swift/SIL/DynamicCasts.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILInstruction.h"
Expand Down Expand Up @@ -5934,7 +5935,11 @@ RValue RValueEmitter::visitMoveExpr(MoveExpr *E, SGFContext C) {
}

// If we aren't loadable, then create a temporary initialization and
// mark_unresolved_move into that.
// mark_unresolved_move into that if we have a copyable type if we have a move
// only, just add a copy_addr init.
//
// The reason why we do this is that we only use mark_unresolved_move_addr and
// the move operator checker for copyable values.
std::unique_ptr<TemporaryInitialization> optTemp;
optTemp = SGF.emitTemporary(E, SGF.getTypeLowering(subType));
SILValue toAddr = optTemp->getAddressForInPlaceInitialization(SGF, E);
Expand All @@ -5945,7 +5950,12 @@ RValue RValueEmitter::visitMoveExpr(MoveExpr *E, SGFContext C) {
SGF.emitRValue(subExpr, SGFContext(SGFContext::AllowImmediatePlusZero))
.getAsSingleValue(SGF, subExpr);
assert(mv.getType().isAddress());
SGF.B.createMarkUnresolvedMoveAddr(subExpr, mv.getValue(), toAddr);
if (mv.getType().isMoveOnly()) {
SGF.B.createCopyAddr(subExpr, mv.getValue(), toAddr, IsNotTake,
IsInitialization);
} else {
SGF.B.createMarkUnresolvedMoveAddr(subExpr, mv.getValue(), toAddr);
}
optTemp->finishInitialization(SGF);
return RValue(SGF, {optTemp->getManagedAddress()}, subType.getASTType());
}
Expand Down
15 changes: 13 additions & 2 deletions lib/SILGen/SILGenLValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "swift/AST/GenericEnvironment.h"
#include "swift/AST/PropertyWrappers.h"
#include "swift/AST/TypeCheckRequests.h"
#include "swift/SIL/Consumption.h"
#include "swift/SIL/InstructionUtils.h"
#include "swift/SIL/MemAccessUtils.h"
#include "swift/SIL/PrettyStackTrace.h"
Expand Down Expand Up @@ -3914,11 +3915,21 @@ LValue SILGenLValue::visitMoveExpr(MoveExpr *e, SGFAccessKind accessKind,

ManagedValue addr = SGF.emitAddressOfLValue(e, std::move(baseLV));

// Now create the temporary and
// Now create the temporary and move our value into there.
auto temp =
SGF.emitFormalAccessTemporary(e, SGF.F.getTypeLowering(addr.getType()));
auto toAddr = temp->getAddressForInPlaceInitialization(SGF, e);
SGF.B.createMarkUnresolvedMoveAddr(e, addr.getValue(), toAddr);

// If we have a move only type, we use a copy_addr that will be handled by the
// address move only checker. If we have a copyable type, we need to use a
// mark_unresolved_move_addr to ensure that the move operator checker performs
// the relevant checking.
if (addr.getType().isMoveOnly()) {
SGF.B.createCopyAddr(e, addr.getValue(), toAddr, IsNotTake,
IsInitialization);
} else {
SGF.B.createMarkUnresolvedMoveAddr(e, addr.getValue(), toAddr);
}
temp->finishInitialization(SGF);

// Now return the temporary in a value component.
Expand Down
28 changes: 19 additions & 9 deletions lib/SILOptimizer/Mandatory/MoveKillsCopyableValuesChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ bool MoveKillsCopyableValuesChecker::check() {
SmallSetVector<SILValue, 32> valuesToCheck;

for (auto *arg : fn->getEntryBlock()->getSILFunctionArguments()) {
if (arg->getOwnershipKind() == OwnershipKind::Owned) {
if (arg->getOwnershipKind() == OwnershipKind::Owned &&
!arg->getType().isMoveOnly()) {
LLVM_DEBUG(llvm::dbgs() << "Found owned arg to check: " << *arg);
valuesToCheck.insert(arg);
}
Expand All @@ -409,7 +410,7 @@ bool MoveKillsCopyableValuesChecker::check() {
for (auto &block : *fn) {
for (auto &ii : block) {
if (auto *bbi = dyn_cast<BeginBorrowInst>(&ii)) {
if (bbi->isLexical()) {
if (bbi->isLexical() && !bbi->getType().isMoveOnly()) {
LLVM_DEBUG(llvm::dbgs()
<< "Found lexical lifetime to check: " << *bbi);
valuesToCheck.insert(bbi);
Expand Down Expand Up @@ -551,19 +552,28 @@ class MoveKillsCopyableValuesCheckerPass : public SILFunctionTransform {
SILAnalysis::InvalidationKind::BranchesAndInstructions);
}

// Now search through our function one last time and any move_value
// [allows_diagnostics] that remain are ones that we did not know how to
// check so emit a diagnostic so the user doesn't assume that they have
// guarantees.
// Now search through our function one last time and:
//
// 1. Given any move_value on a move only type, just unset the allows
// diagnostics flag. The move checker will have emitted any errors caused
// by our move [allows_diagnostic] earlier in the compilation pipeline.
//
// 2. Any move_value [allows_diagnostics] that remain that are not on a move
// only type are ones that we did not know how to check so emit a
// diagnostic so the user doesn't assume that they have guarantees.
//
// TODO: Emit specific diagnostics here (e.x.: _move of global).
if (DisableUnhandledMoveDiagnostic)
return;
for (auto &block : *fn) {
for (auto &inst : block) {
if (auto *mvi = dyn_cast<MoveValueInst>(&inst)) {
if (mvi->getAllowDiagnostics()) {
emitUnsupportedUseCaseError(mvi);
if (mvi->getType().isMoveOnly()) {
mvi->setAllowsDiagnostics(false);
continue;
}

if (!DisableUnhandledMoveDiagnostic)
emitUnsupportedUseCaseError(mvi);
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2061,3 +2061,15 @@ public func closureAndClosureCaptureClassArgUseAfterConsume(_ x2: inout Klass) {
}
f()
}

/////////////////////////////
// Tests For Move Operator //
/////////////////////////////

func moveOperatorTest(_ k: __owned Klass) {
var k2 = k // expected-error {{'k2' consumed more than once}}
k2 = Klass()
let k3 = _move k2 // expected-note {{consuming use}}
let _ = _move k2 // expected-note {{consuming use}}
let _ = k3
}
11 changes: 11 additions & 0 deletions test/SILOptimizer/moveonly_objectchecker_diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2326,3 +2326,14 @@ public func closureAndClosureCaptureClassOwnedArgUseAfterConsume2(_ x2: __owned
f()
print(x2) // expected-note {{consuming use}}
}

/////////////////////////////
// Tests For Move Operator //
/////////////////////////////

func moveOperatorTest(_ k: __owned Klass) {
let k2 = k // expected-error {{'k2' consumed more than once}}
let k3 = _move k2 // expected-note {{consuming use}}
let _ = _move k2 // expected-note {{consuming use}}
let _ = k3
}
11 changes: 11 additions & 0 deletions test/SILOptimizer/noimplicitcopy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2212,3 +2212,14 @@ public func closureAndClosureCaptureClassOwnedArgUseAfterConsume2(@_noImplicitCo
f()
print(x2) // expected-note {{consuming use}}
}

/////////////////////////////
// Tests For Move Operator //
/////////////////////////////

func moveOperatorTest(_ k: __owned Klass) {
@_noImplicitCopy let k2 = k // expected-error {{'k2' consumed more than once}}
@_noImplicitCopy let k3 = _move k2 // expected-note {{consuming use}}
let _ = _move k2 // expected-note {{consuming use}}
let _ = k3
}