Skip to content

Commit e90a7e6

Browse files
jckarterjckarter
committed
SILGen: Fixes for consuming noncopyable optional chaining.
Use the lvalue mechanism to build opaque formal accesses so that they nest properly with writebacks. Don't put a cleanup on the lvalue because that creates a double destroy. Fixes rdar://124362085.
1 parent d887249 commit e90a7e6

File tree

5 files changed

+122
-40
lines changed

5 files changed

+122
-40
lines changed

‎lib/SILGen/SILGenBuilder.cpp‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,28 +1080,6 @@ SILGenBuilder::createOpaqueConsumeBeginAccess(SILLocation loc,
10801080
return SGF.emitManagedRValueWithCleanup(access);
10811081
}
10821082

1083-
ManagedValue
1084-
SILGenBuilder::createFormalAccessOpaqueBorrowBeginAccess(SILLocation loc,
1085-
ManagedValue address) {
1086-
auto access = createBeginAccess(loc, address.getValue(),
1087-
SILAccessKind::Read,
1088-
SILAccessEnforcement::Static,
1089-
/*no nested conflict*/ true, false);
1090-
SGF.Cleanups.pushCleanup<EndAccessCleanup>(access);
1091-
return ManagedValue::forBorrowedAddressRValue(access);
1092-
}
1093-
1094-
ManagedValue
1095-
SILGenBuilder::createFormalAccessOpaqueConsumeBeginAccess(SILLocation loc,
1096-
ManagedValue address) {
1097-
auto access = createBeginAccess(loc, address.forward(SGF),
1098-
SILAccessKind::Deinit,
1099-
SILAccessEnforcement::Static,
1100-
/*no nested conflict*/ true, false);
1101-
SGF.Cleanups.pushCleanup<EndAccessCleanup>(access);
1102-
return SGF.emitFormalAccessManagedRValueWithCleanup(loc, access);
1103-
}
1104-
11051083
ManagedValue
11061084
SILGenBuilder::createBeginBorrow(SILLocation loc, ManagedValue value,
11071085
IsLexical_t isLexical,

‎lib/SILGen/SILGenBuilder.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -470,13 +470,8 @@ class SILGenBuilder : public SILBuilder {
470470

471471
ManagedValue createOpaqueBorrowBeginAccess(SILLocation loc,
472472
ManagedValue address);
473-
ManagedValue createFormalAccessOpaqueBorrowBeginAccess(SILLocation loc,
474-
ManagedValue address);
475-
476473
ManagedValue createOpaqueConsumeBeginAccess(SILLocation loc,
477474
ManagedValue address);
478-
ManagedValue createFormalAccessOpaqueConsumeBeginAccess(SILLocation loc,
479-
ManagedValue address);
480475

481476
using SILBuilder::createBeginBorrow;
482477
ManagedValue createBeginBorrow(

‎lib/SILGen/SILGenExpr.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5422,7 +5422,7 @@ ManagedValue SILGenFunction::emitBindOptional(SILLocation loc,
54225422

54235423
// For move checking purposes, binding always consumes the value whole.
54245424
if (optValue.getType().isMoveOnly() && optValue.getType().isAddress()) {
5425-
optValue = B.createFormalAccessOpaqueConsumeBeginAccess(loc, optValue);
5425+
optValue = B.createOpaqueConsumeBeginAccess(loc, optValue);
54265426
}
54275427

54285428
SILType optValueTy = optValue.getType();

‎lib/SILGen/SILGenLValue.cpp‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,8 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
634634
LValueTypeData typeData,
635635
SGFAccessKind accessKind,
636636
SILAccessEnforcement enforcement,
637-
std::optional<ActorIsolation> actorIso) {
637+
std::optional<ActorIsolation> actorIso,
638+
bool noNestedConflict = false) {
638639
auto silAccessKind = SILAccessKind::Modify;
639640
if (isReadAccess(accessKind))
640641
silAccessKind = SILAccessKind::Read;
@@ -649,7 +650,7 @@ static SILValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
649650

650651
// Enter the access.
651652
addr = SGF.B.createBeginAccess(loc, addr, silAccessKind, enforcement,
652-
/*hasNoNestedConflict=*/false,
653+
noNestedConflict,
653654
/*fromBuiltin=*/false);
654655

655656
// Push a writeback to end it.
@@ -667,10 +668,11 @@ static ManagedValue enterAccessScope(SILGenFunction &SGF, SILLocation loc,
667668
LValueTypeData typeData,
668669
SGFAccessKind accessKind,
669670
SILAccessEnforcement enforcement,
670-
std::optional<ActorIsolation> actorIso) {
671+
std::optional<ActorIsolation> actorIso,
672+
bool noNestedConflict = false) {
671673
return ManagedValue::forLValue(
672-
enterAccessScope(SGF, loc, base, addr.getLValueAddress(), typeData,
673-
accessKind, enforcement, actorIso));
674+
enterAccessScope(SGF, loc, base, addr.getValue(), typeData,
675+
accessKind, enforcement, actorIso, noNestedConflict));
674676
}
675677

676678
// Find the base of the formal access at `address`. If the base requires an
@@ -4373,22 +4375,27 @@ LValue SILGenLValue::visitBindOptionalExpr(BindOptionalExpr *e,
43734375
// Do formal evaluation of the base l-value.
43744376
LValue optLV = visitRec(e->getSubExpr(), baseAccessKind,
43754377
options.forComputedBaseLValue());
4376-
4378+
// For move-checking purposes, the binding is also treated as an opaque use
4379+
// of the entire value, since we can't leave the value partially initialized
4380+
// across multiple optional binding expressions.
43774381
LValueTypeData optTypeData = optLV.getTypeData();
43784382
LValueTypeData valueTypeData =
43794383
getOptionalObjectTypeData(SGF, accessKind, optTypeData);
43804384

43814385
// The chaining operator immediately evaluates the base.
4382-
// For move-checking purposes, the binding is also treated as an opaque use
4383-
// of the entire value, since we can't leave the value partially initialized
4384-
// across multiple optional binding expressions.
4386+
43854387
ManagedValue optBase;
43864388
if (isBorrowAccess(baseAccessKind)) {
43874389
optBase = SGF.emitBorrowedLValue(e, std::move(optLV));
43884390

43894391
if (optBase.getType().isMoveOnly()) {
43904392
if (optBase.getType().isAddress()) {
4391-
optBase = SGF.B.createFormalAccessOpaqueBorrowBeginAccess(e, optBase);
4393+
optBase = enterAccessScope(SGF, e, ManagedValue(),
4394+
optBase, optTypeData,
4395+
baseAccessKind,
4396+
SILAccessEnforcement::Static,
4397+
std::nullopt,
4398+
/*no nested conflict*/ true);
43924399
if (optBase.getType().isLoadable(SGF.F)) {
43934400
optBase = SGF.B.createFormalAccessLoadBorrow(e, optBase);
43944401
}
@@ -4400,9 +4407,14 @@ LValue SILGenLValue::visitBindOptionalExpr(BindOptionalExpr *e,
44004407
} else {
44014408
optBase = SGF.emitAddressOfLValue(e, std::move(optLV));
44024409

4403-
if (isConsumeAccess(accessKind)) {
4410+
if (isConsumeAccess(baseAccessKind)) {
44044411
if (optBase.getType().isMoveOnly()) {
4405-
optBase = SGF.B.createFormalAccessOpaqueConsumeBeginAccess(e, optBase);
4412+
optBase = enterAccessScope(SGF, e, ManagedValue(),
4413+
optBase, optTypeData,
4414+
baseAccessKind,
4415+
SILAccessEnforcement::Static,
4416+
std::nullopt,
4417+
/*no nested conflict*/ true);
44064418
} else {
44074419
// Take ownership of the base.
44084420
optBase = SGF.emitFormalAccessManagedRValueWithCleanup(e,

‎test/SILGen/moveonly_optional_operations_2.swift‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// RUN: %target-swift-frontend -emit-silgen -enable-experimental-feature NoncopyableGenerics -enable-experimental-feature MoveOnlyPartialConsumption -parse-stdlib -module-name Swift %s | %FileCheck %s
2+
3+
@_marker protocol Copyable {}
4+
@_marker protocol Escapable {}
5+
6+
enum Optional<Wrapped: ~Copyable>: ~Copyable {
7+
case none
8+
case some(Wrapped)
9+
}
10+
11+
extension Optional: Copyable where Wrapped: Copyable { }
12+
13+
func _diagnoseUnexpectedNilOptional(_filenameStart: Builtin.RawPointer,
14+
_filenameLength: Builtin.Word,
15+
_filenameIsASCII: Builtin.Int1,
16+
_line: Builtin.Word,
17+
_isImplicitUnwrap: Builtin.Int1) {
18+
}
19+
20+
precedencegroup AssignmentPrecedence {}
21+
22+
struct NC: ~Copyable {
23+
borrowing func b() {}
24+
mutating func m() {}
25+
consuming func c() {}
26+
27+
consuming func c2() -> NC { c2() } // expected-warning{{}}
28+
consuming func c3() -> NCAO { c3() } // expected-warning{{}}
29+
}
30+
31+
struct NCAO: ~Copyable {
32+
var x: Any
33+
34+
borrowing func b() {}
35+
mutating func m() {}
36+
consuming func c() {}
37+
38+
consuming func c2() -> NC { c2() } // expected-warning{{}}
39+
consuming func c3() -> NCAO { c3() } // expected-warning{{}}
40+
}
41+
42+
struct NCPair: ~Copyable {
43+
var first: NC? = .none
44+
var second: NCAO? = .none
45+
}
46+
47+
func consumingSwitchSubject(nc: consuming NC?,
48+
ncao: consuming NCAO?) {
49+
switch nc?.c2() {
50+
default:
51+
break
52+
}
53+
switch ncao?.c2() {
54+
default:
55+
break
56+
}
57+
}
58+
59+
func consumingSwitchSubject2(nc: consuming NC?,
60+
ncao: consuming NCAO?) {
61+
switch nc?.c3() {
62+
default:
63+
break
64+
}
65+
switch ncao?.c3() {
66+
default:
67+
break
68+
}
69+
}
70+
71+
// CHECK-LABEL: sil {{.*}}@${{.*}}23consumingSwitchSubject3
72+
func consumingSwitchSubject3(ncp: consuming NCPair) {
73+
// CHECK: [[PAIR_ACCESS:%.*]] = begin_access [deinit] [unknown] {{.*}} : $*NCPair
74+
// CHECK: [[PAIR_MARK:%.*]] = mark_unresolved_non_copyable_value [assignable_but_not_consumable] [[PAIR_ACCESS]]
75+
// CHECK: [[FIELD:%.*]] = struct_element_addr [[PAIR_MARK]]
76+
// CHECK: [[FIELD_ACCESS:%.*]] = begin_access [deinit] [static] [no_nested_conflict] [[FIELD]]
77+
// CHECK: cond_br {{.*}}, [[SOME_BB:bb[0-9]+]],
78+
// CHECK: [[SOME_BB]]:
79+
// CHECK: [[PAYLOAD_ADDR:%.*]] = unchecked_take_enum_data_addr [[FIELD_ACCESS]]
80+
// CHECK: [[PAYLOAD:%.*]] = load [take] [[PAYLOAD_ADDR]]
81+
// CHECK: apply {{.*}}({{.*}}, [[PAYLOAD]]) : ${{.*}} (@owned NC)
82+
// CHECK-NOT: destroy_addr
83+
// CHECK-NOT: destroy_value
84+
// CHECK: end_access [[FIELD_ACCESS]]
85+
// CHECK-NEXT: end_access [[PAIR_ACCESS]]
86+
// CHECK-NEXT: inject_enum_addr
87+
// CHECK-NEXT: br
88+
89+
switch ncp.first?.c3() {
90+
default:
91+
break
92+
}
93+
switch ncp.second?.c3() {
94+
default:
95+
break
96+
}
97+
}

0 commit comments

Comments
 (0)