Skip to content

[move-only] Fix class setters of address only move only types. #66255

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.

Sign up for GitHub

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 1 commit into from
Jun 1, 2023
Merged

[move-only] Fix class setters of address only move only types. #66255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 7 additions & 5 deletions lib/SILGen/SILGenBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,11 +980,12 @@ ManagedValue SILGenBuilder::createBeginBorrow(SILLocation loc,
return ManagedValue::forUnmanaged(newValue);
}

ManagedValue SILGenBuilder::createMoveValue(SILLocation loc,
ManagedValue value) {
ManagedValue SILGenBuilder::createMoveValue(SILLocation loc, ManagedValue value,
bool isLexical) {
assert(value.isPlusOne(SGF) && "Must be +1 to be moved!");
CleanupCloner cloner(*this, value);
auto *mdi = createMoveValue(loc, value.forward(getSILGenFunction()));
auto *mdi =
createMoveValue(loc, value.forward(getSILGenFunction()), isLexical);
return cloner.clone(mdi);
}

Expand Down Expand Up @@ -1027,8 +1028,9 @@ ManagedValue SILGenBuilder::createGuaranteedCopyableToMoveOnlyWrapperValue(
ManagedValue
SILGenBuilder::createMarkMustCheckInst(SILLocation loc, ManagedValue value,
MarkMustCheckInst::CheckKind kind) {
assert((value.isPlusOne(SGF) || value.isLValue()) &&
"Argument must be at +1 or be an inout!");
assert((value.isPlusOne(SGF) || value.isLValue() ||
value.getType().isAddress()) &&
"Argument must be at +1 or be an address!");
CleanupCloner cloner(*this, value);
auto *mdi = SILBuilder::createMarkMustCheckInst(
loc, value.forward(getSILGenFunction()), kind);
Expand Down
3 changes: 2 additions & 1 deletion lib/SILGen/SILGenBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ class SILGenBuilder : public SILBuilder {
bool isLexical = false);

using SILBuilder::createMoveValue;
ManagedValue createMoveValue(SILLocation loc, ManagedValue value);
ManagedValue createMoveValue(SILLocation loc, ManagedValue value,
bool isLexical = false);

using SILBuilder::createOwnedMoveOnlyWrapperToCopyableValue;
ManagedValue createOwnedMoveOnlyWrapperToCopyableValue(SILLocation loc,
Expand Down
125 changes: 61 additions & 64 deletions lib/SILGen/SILGenProlog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,23 +631,23 @@ class ArgumentInitHelper {
}

void updateArgumentValueForBinding(ManagedValue argrv, SILLocation loc,
ParamDecl *pd, SILValue value,
ParamDecl *pd,
const SILDebugVariable &varinfo) {
bool calledCompletedUpdate = false;
SWIFT_DEFER {
assert(calledCompletedUpdate && "Forgot to call completed update along "
"all paths or manually turn it off");
};
auto completeUpdate = [&](SILValue value) -> void {
SGF.B.createDebugValue(loc, value, varinfo);
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value);
auto completeUpdate = [&](ManagedValue value) -> void {
SGF.B.createDebugValue(loc, value.getValue(), varinfo);
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value.getValue());
calledCompletedUpdate = true;
};

// If we do not need to support lexical lifetimes, just return value as the
// updated value.
if (!SGF.getASTContext().SILOpts.supportsLexicalLifetimes(SGF.getModule()))
return completeUpdate(value);
return completeUpdate(argrv);

// Look for the following annotations on the function argument:
// - @noImplicitCopy
Expand All @@ -659,15 +659,15 @@ class ArgumentInitHelper {
// we need to use copyable to move only to convert it to its move only
// form.
if (!isNoImplicitCopy) {
if (!value->getType().isMoveOnly()) {
if (!argrv.getType().isMoveOnly()) {
// Follow the normal path. The value's lifetime will be enforced based
// on its ownership.
return completeUpdate(value);
return completeUpdate(argrv);
}

// At this point, we have a noncopyable type. If it is owned, create an
// alloc_box for it.
if (value->getOwnershipKind() == OwnershipKind::Owned) {
if (argrv.getOwnershipKind() == OwnershipKind::Owned) {
// TODO: Once owned values are mutable, this needs to become mutable.
auto boxType = SGF.SGM.Types.getContextBoxTypeForCapture(
pd,
Expand Down Expand Up @@ -696,17 +696,18 @@ class ArgumentInitHelper {
// misleading consuming message. We still are able to pass it to
// non-escaping closures though since the onstack partial_apply does not
// consume the value.
assert(value->getOwnershipKind() == OwnershipKind::Guaranteed);
value = SGF.B.createCopyValue(loc, value);
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
SGF.emitManagedRValueWithCleanup(value);
return completeUpdate(value);
assert(argrv.getOwnershipKind() == OwnershipKind::Guaranteed);
argrv = argrv.copy(SGF, loc);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
return completeUpdate(argrv);
}

if (value->getType().isTrivial(SGF.F)) {
value = SGF.B.createOwnedCopyableToMoveOnlyWrapperValue(loc, value);
value = SGF.B.createMoveValue(loc, value, /*isLexical=*/true);
if (argrv.getType().isTrivial(SGF.F)) {
SILValue value = SGF.B.createOwnedCopyableToMoveOnlyWrapperValue(
loc, argrv.getValue());
argrv = SGF.emitManagedRValueWithCleanup(value);
argrv = SGF.B.createMoveValue(loc, argrv, /*isLexical=*/true);

// If our argument was owned, we use no implicit copy. Otherwise, we
// use no copy.
Expand All @@ -723,40 +724,35 @@ class ArgumentInitHelper {
break;
}

value = SGF.B.createMarkMustCheckInst(loc, value, kind);
SGF.emitManagedRValueWithCleanup(value);
return completeUpdate(value);
argrv = SGF.B.createMarkMustCheckInst(loc, argrv, kind);
return completeUpdate(argrv);
}

if (value->getOwnershipKind() == OwnershipKind::Guaranteed) {
value = SGF.B.createGuaranteedCopyableToMoveOnlyWrapperValue(loc, value);
value = SGF.B.createCopyValue(loc, value);
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
SGF.emitManagedRValueWithCleanup(value);
return completeUpdate(value);
if (argrv.getOwnershipKind() == OwnershipKind::Guaranteed) {
argrv = SGF.B.createGuaranteedCopyableToMoveOnlyWrapperValue(loc, argrv);
argrv = argrv.copy(SGF, loc);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
return completeUpdate(argrv);
}

if (value->getOwnershipKind() == OwnershipKind::Owned) {
if (argrv.getOwnershipKind() == OwnershipKind::Owned) {
// If we have an owned value, forward it into the mark_must_check to
// avoid an extra destroy_value.
value = SGF.B.createOwnedCopyableToMoveOnlyWrapperValue(
loc, argrv.forward(SGF));
value = SGF.B.createMoveValue(loc, value, true /*is lexical*/);
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
SGF.emitManagedRValueWithCleanup(value);
return completeUpdate(value);
argrv = SGF.B.createOwnedCopyableToMoveOnlyWrapperValue(loc, argrv);
argrv = SGF.B.createMoveValue(loc, argrv, true /*is lexical*/);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
return completeUpdate(argrv);
}

return completeUpdate(value);
return completeUpdate(argrv);
}

/// Create a SILArgument and store its value into the given Initialization,
/// if not null.
void makeArgumentIntoBinding(SILLocation loc, ParamDecl *pd) {
ManagedValue argrv = makeArgument(loc, pd);
SILValue value = argrv.getValue();
if (pd->isInOut()) {
assert(argrv.getType().isAddress() && "expected inout to be address");
} else if (!pd->isImmutableInFunctionBody()) {
Expand All @@ -774,33 +770,34 @@ class ArgumentInitHelper {
SILDebugVariable varinfo(pd->isImmutableInFunctionBody(), ArgNo);
if (!argrv.getType().isAddress()) {
// NOTE: We setup SGF.VarLocs[pd] in updateArgumentValueForBinding.
updateArgumentValueForBinding(argrv, loc, pd, value, varinfo);
updateArgumentValueForBinding(argrv, loc, pd, varinfo);
return;
}

if (auto *allocStack = dyn_cast<AllocStackInst>(value)) {
if (auto *allocStack = dyn_cast<AllocStackInst>(argrv.getValue())) {
allocStack->setArgNo(ArgNo);
if (SGF.getASTContext().SILOpts.supportsLexicalLifetimes(
SGF.getModule()) &&
SGF.F.getLifetime(pd, value->getType()).isLexical())
SGF.F.getLifetime(pd, allocStack->getType()).isLexical())
allocStack->setIsLexical();
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value);
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(allocStack);
return;
}

SILValue debugOperand = value;
if (value->getType().isMoveOnly()) {
SILValue debugOperand = argrv.getValue();

if (argrv.getType().isMoveOnly()) {
switch (pd->getValueOwnership()) {
case ValueOwnership::Default:
if (pd->isSelfParameter()) {
assert(!isa<MarkMustCheckInst>(value) &&
assert(!isa<MarkMustCheckInst>(argrv.getValue()) &&
"Should not have inserted mark must check inst in EmitBBArgs");
if (!pd->isInOut()) {
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
}
} else {
if (auto *fArg = dyn_cast<SILFunctionArgument>(value)) {
if (auto *fArg = dyn_cast<SILFunctionArgument>(argrv.getValue())) {
switch (fArg->getArgumentConvention()) {
case SILArgumentConvention::Direct_Guaranteed:
case SILArgumentConvention::Direct_Owned:
Expand All @@ -814,51 +811,51 @@ class ArgumentInitHelper {
case SILArgumentConvention::Pack_Out:
llvm_unreachable("Should have been handled elsewhere");
case SILArgumentConvention::Indirect_In:
value = SGF.B.createMarkMustCheckInst(
loc, value,
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv,
MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
break;
case SILArgumentConvention::Indirect_In_Guaranteed:
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
}
} else {
assert(isa<MarkMustCheckInst>(value) &&
assert(isa<MarkMustCheckInst>(argrv.getValue()) &&
"Should have inserted mark must check inst in EmitBBArgs");
}
}
break;
case ValueOwnership::InOut: {
assert(isa<MarkMustCheckInst>(value)
&& "Expected mark must check inst with inout to be handled in "
"emitBBArgs earlier");
auto mark = cast<MarkMustCheckInst>(value);
assert(isa<MarkMustCheckInst>(argrv.getValue()) &&
"Expected mark must check inst with inout to be handled in "
"emitBBArgs earlier");
auto mark = cast<MarkMustCheckInst>(argrv.getValue());
debugOperand = mark->getOperand();
break;
}
case ValueOwnership::Owned:
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::ConsumableAndAssignable);
break;
case ValueOwnership::Shared:
value = SGF.B.createMarkMustCheckInst(
loc, value, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
argrv = SGF.B.createMarkMustCheckInst(
loc, argrv, MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
break;
}
}

DebugValueInst *debugInst
= SGF.B.createDebugValueAddr(loc, debugOperand, varinfo);
if (value != debugOperand) {
if (auto valueInst = dyn_cast<MarkMustCheckInst>(value)) {

if (argrv.getValue() != debugOperand) {
if (auto valueInst = dyn_cast<MarkMustCheckInst>(argrv.getValue())) {
// Move the debug instruction outside of any marker instruction that might
// have been applied to the value, so that analysis doesn't move the
// debug_value anywhere it shouldn't be.
debugInst->moveBefore(valueInst);
}
}
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(value);
SGF.VarLocs[pd] = SILGenFunction::VarLoc::get(argrv.getValue());
}

void emitParam(ParamDecl *PD) {
Expand Down
25 changes: 22 additions & 3 deletions test/SILGen/moveonly.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,9 +988,9 @@ func testConditionallyInitializedLet() {
consumeVal(x)
}

/////////////////////////////
// MARK: AddressOnlySetter //
/////////////////////////////
////////////////////////
// MARK: Setter Tests //
////////////////////////

struct AddressOnlySetterTester : ~Copyable {
var a: AddressOnlyProtocol {
Expand All @@ -1005,6 +1005,24 @@ struct AddressOnlySetterTester : ~Copyable {
}
}

public class NonFinalClassTest {
// CHECK: sil hidden [transparent] [ossa] @$s8moveonly17NonFinalClassTestC1xAA19AddressOnlyProtocolVvs : $@convention(method) (@in AddressOnlyProtocol, @guaranteed NonFinalClassTest) -> () {
// CHECK: bb0([[INPUT:%.*]] : $*AddressOnlyProtocol, [[SELF:%.*]] : @guaranteed
// CHECK: [[MARK:%.*]] = mark_must_check [consumable_and_assignable] [[INPUT]]
// CHECK: [[TEMP:%.*]] = alloc_stack $AddressOnlyProtocol
// CHECK: copy_addr [[MARK]] to [init] [[TEMP]]
// CHECK: [[REF:%.*]] = ref_element_addr [[SELF]]
// CHECK: [[ACCESS:%.*]] = begin_access [modify] [dynamic] [[REF]]
// CHECK: [[MARK2:%.*]] = mark_must_check [assignable_but_not_consumable] [[ACCESS]]
// CHECK: copy_addr [take] [[TEMP]] to [[MARK2]]
// CHECK: end_access [[ACCESS]]
// CHECK: dealloc_stack [[TEMP]]
// CHECK: destroy_addr [[MARK]]
// CHECK: } // end sil function '$s8moveonly17NonFinalClassTestC1xAA19AddressOnlyProtocolVvs'
var x: AddressOnlyProtocol
init(y: consuming AddressOnlyProtocol) { self.x = y }
}

/////////////////////
// MARK: Subscript //
/////////////////////
Expand Down Expand Up @@ -3071,3 +3089,4 @@ public func testSubscriptGetModifyThroughParentClass_BaseLoadable_ResultAddressO
m.computedTester2[0].nonMutatingFunc()
m.computedTester2[0].mutatingFunc()
}

11 changes: 11 additions & 0 deletions test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4486,3 +4486,14 @@ func testMyEnum() {
_ = consume x // expected-note {{consumed again here}}
}
}

////////////////////////
// MARK: Setter Tests //
////////////////////////

public class NonFinalCopyableKlassWithMoveOnlyField {
var moveOnlyVarStruct = NonTrivialStruct()
let moveOnlyLetStruct = NonTrivialStruct()
var moveOnlyVarProt = AddressOnlyProtocol()
let moveOnlyLetProt = AddressOnlyProtocol()
}