Skip to content

[5.10][SILGen/DI] InitAccessors: Fix handling of nonmutating set when type has other stored properties #69104

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
12 changes: 8 additions & 4 deletions lib/SILGen/SILGenFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1820,8 +1820,8 @@ SILGenFunction::emitApplyOfSetterToBase(SILLocation loc, SILDeclRef setter,
SmallVector<ManagedValue, 4> captures;
emitCaptures(loc, setter, CaptureEmission::AssignByWrapper, captures);

for (auto capture : captures)
capturedArgs.push_back(capture.forward(*this));
llvm::transform(captures, std::back_inserter(capturedArgs),
[](auto &capture) { return capture.getValue(); });
} else {
assert(base);

Expand All @@ -1839,16 +1839,20 @@ SILGenFunction::emitApplyOfSetterToBase(SILLocation loc, SILDeclRef setter,
// nonmutating setter, for example.
capturedBase = B.createTrivialLoadOr(loc, base.getValue(),
LoadOwnershipQualifier::Copy);
// On-stack partial apply doesn't take ownership of the base, so
// we have to destroy it manually.
enterDestroyCleanup(capturedBase);
} else {
capturedBase = base.copy(*this, loc).forward(*this);
capturedBase = base.borrow(*this, loc).getValue();
}

capturedArgs.push_back(capturedBase);
}

PartialApplyInst *setterPAI =
B.createPartialApply(loc, setterFRef, substitutions, capturedArgs,
ParameterConvention::Direct_Guaranteed);
ParameterConvention::Direct_Guaranteed,
PartialApplyInst::OnStackKind::OnStack);
return emitManagedRValueWithCleanup(setterPAI).getValue();
}

Expand Down
34 changes: 26 additions & 8 deletions lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -662,14 +662,32 @@ bool LifetimeChecker::shouldEmitError(const SILInstruction *Inst) {
// This is safe to ignore because assign_by_wrapper/assign_or_init will
// only be re-written to use the setter if the value is fully initialized.
if (auto *load = dyn_cast<SingleValueInstruction>(Inst)) {
if (auto Op = load->getSingleUse()) {
if (auto PAI = dyn_cast<PartialApplyInst>(Op->getUser())) {
if (std::find_if(PAI->use_begin(), PAI->use_end(), [](auto PAIUse) {
return isa<AssignByWrapperInst>(PAIUse->getUser()) ||
isa<AssignOrInitInst>(PAIUse->getUser());
}) != PAI->use_end()) {
return false;
}
auto isOnlyUsedByPartialApply =
[&](const SingleValueInstruction *inst) -> PartialApplyInst * {
Operand *result = nullptr;
for (auto *op : inst->getUses()) {
auto *user = op->getUser();

// Ignore copies, destroys and borrows because they'd be
// erased together with the setter.
if (isa<DestroyValueInst>(user) || isa<CopyValueInst>(user) ||
isa<BeginBorrowInst>(user) || isa<EndBorrowInst>(user))
continue;

if (result)
return nullptr;

result = op;
}
return result ? dyn_cast<PartialApplyInst>(result->getUser()) : nullptr;
};

if (auto *PAI = isOnlyUsedByPartialApply(load)) {
if (std::find_if(PAI->use_begin(), PAI->use_end(), [](auto PAIUse) {
return isa<AssignByWrapperInst>(PAIUse->getUser()) ||
isa<AssignOrInitInst>(PAIUse->getUser());
}) != PAI->use_end()) {
return false;
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion test/Interpreter/init_accessors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -865,6 +865,7 @@ do {

struct TestNonMutatingSetNoDefault {
var _count: Int
var _other: String = ""

var count: Int {
@storageRestrictions(initializes: _count)
Expand Down Expand Up @@ -917,7 +918,7 @@ do {
// CHECK-NEXT: test-nonmutating-set-2: TestNonMutatingSetDefault(_count: 0)
// CHECK-NEXT: init accessor is called: -1
// CHECK-NEXT: nonmutating set called: 0
// CHECK-NEXT: test-nonmutating-set-3: TestNonMutatingSetNoDefault(_count: -1)
// CHECK-NEXT: test-nonmutating-set-3: TestNonMutatingSetNoDefault(_count: -1, _other: "")
// CHECK-NEXT: init accessor is called: 42
// CHECK-NEXT: nonmutating set called: 0
// CHECK-NEXT: test-nonmutating-set-4: TestNonMutatingSetCustom(_count: 42)
8 changes: 4 additions & 4 deletions test/SILGen/objc_properties.swift
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,11 @@ class SomeWrapperTests {

// CHECK-LABEL: sil hidden [ossa] @$s15objc_properties16SomeWrapperTestsCyACSScfc : $@convention(method) (@owned String, @owned SomeWrapperTests) -> @owned SomeWrapperTests {
// CHECK: [[M:%.*]] = function_ref @$s15objc_properties16SomeWrapperTestsC04someD0SivsTD
// CHECK: [[C:%.*]] = partial_apply [callee_guaranteed] [[M]]({{.*}})
// CHECK: assign_by_wrapper {{%.*}}: $Int to {{%.*}} : $*SomeWrapper, init {{.*}} : $@callee_guaranteed (Int) -> SomeWrapper, set [[C]] : $@callee_guaranteed (Int) -> ()
// CHECK: [[C:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[M]]({{.*}})
// CHECK: assign_by_wrapper {{%.*}}: $Int to {{%.*}} : $*SomeWrapper, init {{.*}} : $@callee_guaranteed (Int) -> SomeWrapper, set [[C]] : $@noescape @callee_guaranteed (Int) -> ()
// CHECK: [[M:%.*]] = function_ref @$s15objc_properties16SomeWrapperTestsC1sSSSgvsTD
// CHECK: [[C:%.*]] = partial_apply [callee_guaranteed] [[M]](
// CHECK: assign_by_wrapper {{.*}} : $Optional<String> to {{.*}} : $*W<Optional<String>>, init {{.*}} : $@callee_guaranteed (@owned Optional<String>) -> @owned W<Optional<String>>, set [[C]] : $@callee_guaranteed (@owned Optional<String>) -> ()
// CHECK: [[C:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[M]](
// CHECK: assign_by_wrapper {{.*}} : $Optional<String> to {{.*}} : $*W<Optional<String>>, init {{.*}} : $@callee_guaranteed (@owned Optional<String>) -> @owned W<Optional<String>>, set [[C]] : $@noescape @callee_guaranteed (@owned Optional<String>) -> ()
init(_ s: String) {
someWrapper = 1000
self.s = s
Expand Down
4 changes: 2 additions & 2 deletions test/SILGen/property_wrapper_local.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ func testLocalWrapper() {
// CHECK: [[S:%.*]] = function_ref @$s22property_wrapper_local16testLocalWrapperyyF5valueL_Sivs : $@convention(thin) (Int, @guaranteed { var Wrapper<Int> }) -> ()
// CHECK-NEXT: [[C:%.*]] = copy_value [[W]] : ${ var Wrapper<Int> }
// CHECK-NOT: mark_function_escape
// CHECK-NEXT: [[SPA:%.*]] = partial_apply [callee_guaranteed] [[S]]([[C]]) : $@convention(thin) (Int, @guaranteed { var Wrapper<Int> }) -> ()
// CHECK-NEXT: assign_by_wrapper {{%.*}} : $Int to [[P]] : $*Wrapper<Int>, init [[IPA]] : $@callee_guaranteed (Int) -> Wrapper<Int>, set [[SPA]] : $@callee_guaranteed (Int) -> ()
// CHECK-NEXT: [[SPA:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[S]]([[C]]) : $@convention(thin) (Int, @guaranteed { var Wrapper<Int> }) -> ()
// CHECK-NEXT: assign_by_wrapper {{%.*}} : $Int to [[P]] : $*Wrapper<Int>, init [[IPA]] : $@callee_guaranteed (Int) -> Wrapper<Int>, set [[SPA]] : $@noescape @callee_guaranteed (Int) -> ()

_ = value
// CHECK: mark_function_escape [[P]] : $*Wrapper<Int>
Expand Down
4 changes: 2 additions & 2 deletions test/SILGen/property_wrappers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -903,8 +903,8 @@ struct NonMutatingSetterWrapper<Value> {
struct NonMutatingWrapperTestStruct {
// CHECK-LABEL: sil hidden [ossa] @$s17property_wrappers28NonMutatingWrapperTestStructV3valACSi_tcfC : $@convention(method) (Int, @thin NonMutatingWrapperTestStruct.Type) -> NonMutatingWrapperTestStruct {
// CHECK: %[[LOAD:[0-9]+]] = load [trivial] %[[SRC:[0-9]+]] : $*NonMutatingWrapperTestStruct
// CHECK-NEXT: %[[SET_PA:[0-9]+]] = partial_apply [callee_guaranteed] %[[PW_SETTER:[0-9]+]](%[[LOAD]]) : $@convention(method) (Int, NonMutatingWrapperTestStruct) -> ()
// CHECK-NEXT: assign_by_wrapper %[[SETVAL:[0-9]+]] : $Int to %[[ADDR:[0-9]+]] : $*NonMutatingSetterWrapper<Int>, init %[[INIT_PA:[0-9]+]] : $@callee_guaranteed (Int) -> NonMutatingSetterWrapper<Int>, set %[[SET_PA]] : $@callee_guaranteed (Int) -> ()
// CHECK-NEXT: %[[SET_PA:[0-9]+]] = partial_apply [callee_guaranteed] [on_stack] %[[PW_SETTER:[0-9]+]](%[[LOAD]]) : $@convention(method) (Int, NonMutatingWrapperTestStruct) -> ()
// CHECK-NEXT: assign_by_wrapper %[[SETVAL:[0-9]+]] : $Int to %[[ADDR:[0-9]+]] : $*NonMutatingSetterWrapper<Int>, init %[[INIT_PA:[0-9]+]] : $@callee_guaranteed (Int) -> NonMutatingSetterWrapper<Int>, set %[[SET_PA]] : $@noescape @callee_guaranteed (Int) -> ()
@NonMutatingSetterWrapper var SomeProp: Int
init(val: Int) {
SomeProp = val
Expand Down
8 changes: 4 additions & 4 deletions test/SILGen/resilient_assign_by_wrapper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ public class AddressOnlySetter {

// CHECK: [[E2:%.*]] = alloc_stack $AddressOnlyEnum
// CHECK-NEXT: inject_enum_addr [[E2]] : $*AddressOnlyEnum, #AddressOnlyEnum.some!enumelt
// CHECK: [[S:%.*]] = partial_apply [callee_guaranteed] {{%.*}}({{%.*}}) : $@convention(method) (@in AddressOnlyEnum, @guaranteed AddressOnlySetter) -> ()
// CHECK: [[S:%.*]] = partial_apply [callee_guaranteed] [on_stack] {{%.*}}({{%.*}}) : $@convention(method) (@in AddressOnlyEnum, @guaranteed AddressOnlySetter) -> ()
// CHECK: assign_by_wrapper [[E2]] : $*AddressOnlyEnum
// CHECK-SAME: set [[S]] : $@callee_guaranteed (@in AddressOnlyEnum) -> ()
// CHECK-SAME: set [[S]] : $@noescape @callee_guaranteed (@in AddressOnlyEnum) -> ()
self.value = .some
}

Expand All @@ -60,7 +60,7 @@ extension SubstitutedSetter where T == Bool {
// CHECK: [[B:%.*]] = alloc_stack $Bool
// CHECK: assign_by_wrapper [[B]] : $*Bool to [[W]] : $*WrapGod<Bool>
// CHECK-SAME: init {{%.*}} : $@callee_guaranteed (@in Bool) -> @out WrapGod<Bool>
// CHECK-SAME: set {{%.*}} : $@callee_guaranteed (@in Bool) -> ()
// CHECK-SAME: set {{%.*}} : $@noescape @callee_guaranteed (@in Bool) -> ()
self.value = true
}
}
Expand Down Expand Up @@ -96,7 +96,7 @@ extension ObjectifiedSetter where T == SomeObject {
// CHECK: [[STORAGE:%.*]] = struct_element_addr {{%.*}} : $*ObjectifiedSetter<SomeObject>, #ObjectifiedSetter._value
// CHECK: assign_by_wrapper [[OBJ]] : $SomeObject to [[STORAGE]] : $*WrapGod<SomeObject>
// CHECK-SAME: init {{%.*}} : $@callee_guaranteed (@owned SomeObject) -> @out WrapGod<SomeObject>
// CHECK-SAME: set {{%.*}} : $@callee_guaranteed (@owned SomeObject) -> ()
// CHECK-SAME: set {{%.*}} : $@noescape @callee_guaranteed (@owned SomeObject) -> ()
self.value = SomeObject()
}
}
Expand Down
Loading