Skip to content

Commit 87ca878

Browse files
gottesmmgottesmm
committed
[silgen] Fix another tuple explosion -> borrow + copy since we do not have the destructure operation yet.
rdar://29791263
1 parent 6920290 commit 87ca878

File tree

12 files changed

+197
-62
lines changed

12 files changed

+197
-62
lines changed

‎lib/SIL/SILValue.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ CONSTANT_OWNERSHIP_INST(Owned, PartialApply)
215215
CONSTANT_OWNERSHIP_INST(Owned, StrongPin)
216216
CONSTANT_OWNERSHIP_INST(Owned, ThinToThickFunction)
217217
CONSTANT_OWNERSHIP_INST(Owned, InitExistentialOpaque)
218+
CONSTANT_OWNERSHIP_INST(Owned, UnconditionalCheckedCastOpaque)
218219

219220
// One would think that these /should/ be unowned. In truth they are owned since
220221
// objc metatypes do not go through the retain/release fast path. In their
@@ -433,7 +434,6 @@ FORWARDING_OWNERSHIP_INST(Struct)
433434
FORWARDING_OWNERSHIP_INST(Tuple)
434435
FORWARDING_OWNERSHIP_INST(UncheckedRefCast)
435436
FORWARDING_OWNERSHIP_INST(UnconditionalCheckedCast)
436-
FORWARDING_OWNERSHIP_INST(UnconditionalCheckedCastOpaque)
437437
FORWARDING_OWNERSHIP_INST(Upcast)
438438
FORWARDING_OWNERSHIP_INST(MarkUninitialized)
439439
FORWARDING_OWNERSHIP_INST(UncheckedEnumData)

‎lib/SIL/SILVerifier.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,14 +1260,18 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
12601260
"store [init] or store [assign] can only be applied to "
12611261
"non-trivial types");
12621262
break;
1263-
case StoreOwnershipQualifier::Trivial:
1263+
case StoreOwnershipQualifier::Trivial: {
12641264
require(
12651265
F.hasQualifiedOwnership(),
12661266
"Inst with qualified ownership in a function that is not qualified");
1267-
require(SI->getSrc()->getType().isTrivial(SI->getModule()),
1268-
"A store with trivial ownership must store a trivial type");
1267+
SILValue Src = SI->getSrc();
1268+
require(Src->getType().isTrivial(SI->getModule()) ||
1269+
Src.getOwnershipKind() == ValueOwnershipKind::Trivial,
1270+
"A store with trivial ownership must store a type with trivial "
1271+
"ownership");
12691272
break;
12701273
}
1274+
}
12711275
}
12721276

12731277
void checkAssignInst(AssignInst *AI) {

‎lib/SILGen/ManagedValue.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,9 @@ class ConsumableManagedValue {
348348

349349
SILType getType() const { return Value.getType(); }
350350
SILValue getValue() const { return Value.getValue(); }
351+
ValueOwnershipKind getOwnershipKind() const {
352+
return Value.getOwnershipKind();
353+
}
351354

352355
/// Return a managed value appropriate for the final use of this CMV.
353356
ManagedValue getFinalManagedValue() const { return Value; }

‎lib/SILGen/RValue.cpp

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,33 @@ class ExplodeTupleValue
8181
values.push_back(v);
8282
}
8383

84-
void visitTupleType(CanTupleType tupleFormalType, ManagedValue tupleMV) {
84+
void visitObjectTupleType(CanTupleType tupleFormalType, ManagedValue tuple) {
85+
bool isPlusZero = tuple.isPlusZeroRValueOrTrivial();
86+
// SEMANTIC ARC TODO: This needs to be a take.
87+
tuple = tuple.borrow(gen, loc);
88+
89+
for (auto i : indices(tupleFormalType->getElements())) {
90+
CanType eltFormalType = tupleFormalType.getElementType(i);
91+
assert(eltFormalType->isMaterializable());
92+
93+
auto eltTy = tuple.getType().getTupleElementType(i);
94+
assert(eltTy.isAddress() == tuple.getType().isAddress());
95+
auto &eltTI = gen.getTypeLowering(eltTy);
96+
97+
// Project the element.
98+
assert(eltTI.isLoadable() || !gen.silConv.useLoweredAddresses());
99+
ManagedValue elt = gen.B.createTupleExtract(loc, tuple, i, eltTy);
100+
// If we're returning a +1 value, emit a cleanup for the member
101+
// to cover for the cleanup we disabled for the tuple aggregate.
102+
if (!isPlusZero)
103+
elt = gen.B.createCopyValue(loc, elt);
104+
105+
visit(eltFormalType, elt);
106+
}
107+
}
108+
109+
void visitAddressTupleType(CanTupleType tupleFormalType,
110+
ManagedValue tupleMV) {
85111
bool isPlusZero = tupleMV.isPlusZeroRValueOrTrivial();
86112
SILValue tuple = tupleMV.forward(gen);
87113

@@ -94,19 +120,13 @@ class ExplodeTupleValue
94120
auto &eltTI = gen.getTypeLowering(eltTy);
95121

96122
// Project the element.
97-
SILValue elt;
98-
if (tuple->getType().isObject()) {
99-
assert(eltTI.isLoadable() || !gen.silConv.useLoweredAddresses());
100-
elt = gen.B.createTupleExtract(loc, tuple, i, eltTy);
101-
} else {
102-
elt = gen.B.createTupleElementAddr(loc, tuple, i, eltTy);
103-
104-
// RValue has an invariant that loadable values have been
105-
// loaded. Except it's not really an invariant, because
106-
// argument emission likes to lie sometimes.
107-
if (eltTI.isLoadable()) {
108-
elt = eltTI.emitLoad(gen.B, loc, elt, LoadOwnershipQualifier::Take);
109-
}
123+
SILValue elt = gen.B.createTupleElementAddr(loc, tuple, i, eltTy);
124+
125+
// RValue has an invariant that loadable values have been
126+
// loaded. Except it's not really an invariant, because
127+
// argument emission likes to lie sometimes.
128+
if (eltTI.isLoadable()) {
129+
elt = eltTI.emitLoad(gen.B, loc, elt, LoadOwnershipQualifier::Take);
110130
}
111131

112132
// If we're returning a +1 value, emit a cleanup for the member
@@ -117,6 +137,14 @@ class ExplodeTupleValue
117137
visit(eltFormalType, eltMV);
118138
}
119139
}
140+
141+
void visitTupleType(CanTupleType tupleFormalType, ManagedValue tuple) {
142+
if (tuple.getType().isObject()) {
143+
return visitObjectTupleType(tupleFormalType, tuple);
144+
}
145+
146+
visitAddressTupleType(tupleFormalType, tuple);
147+
}
120148
};
121149

122150
enum class ImplodeKind { Unmanaged, Forward, Copy };

‎lib/SILGen/SILGenBuilder.cpp

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -534,3 +534,19 @@ ManagedValue SILGenBuilder::createUpcast(SILLocation Loc, ManagedValue Original,
534534

535535
return gen.emitManagedRValueWithCleanup(convertedValue);
536536
}
537+
538+
ManagedValue SILGenBuilder::createTupleElementAddr(SILLocation Loc,
539+
ManagedValue Base,
540+
unsigned Index,
541+
SILType Type) {
542+
SILValue TupleEltAddr =
543+
SILBuilder::createTupleElementAddr(Loc, Base.getValue(), Index, Type);
544+
return ManagedValue::forUnmanaged(TupleEltAddr);
545+
}
546+
547+
ManagedValue SILGenBuilder::createTupleElementAddr(SILLocation Loc,
548+
ManagedValue Value,
549+
unsigned Index) {
550+
SILType Type = Value.getType().getTupleElementType(Index);
551+
return createTupleElementAddr(Loc, Value, Index, Type);
552+
}

‎lib/SILGen/SILGenBuilder.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,15 @@ class SILGenBuilder : public SILBuilder {
163163
ArrayRef<ManagedValue> elementCountOperands);
164164

165165
using SILBuilder::createTupleExtract;
166-
167166
ManagedValue createTupleExtract(SILLocation loc, ManagedValue value,
168167
unsigned index, SILType type);
169168
ManagedValue createTupleExtract(SILLocation loc, ManagedValue value,
170169
unsigned index);
170+
using SILBuilder::createTupleElementAddr;
171+
ManagedValue createTupleElementAddr(SILLocation loc, ManagedValue addr,
172+
unsigned index, SILType type);
173+
ManagedValue createTupleElementAddr(SILLocation loc, ManagedValue addr,
174+
unsigned index);
171175

172176
using SILBuilder::createLoadBorrow;
173177
ManagedValue createLoadBorrow(SILLocation loc, ManagedValue base);
@@ -196,9 +200,9 @@ class SILGenBuilder : public SILBuilder {
196200
ManagedValue createLoadTake(SILLocation loc, ManagedValue addr);
197201
ManagedValue createLoadTake(SILLocation loc, ManagedValue addr,
198202
const TypeLowering &lowering);
199-
ManagedValue createLoadCopy(SILLocation Loc, ManagedValue Addr);
200-
ManagedValue createLoadCopy(SILLocation Loc, ManagedValue Addr,
201-
const TypeLowering &Lowering);
203+
ManagedValue createLoadCopy(SILLocation loc, ManagedValue addr);
204+
ManagedValue createLoadCopy(SILLocation loc, ManagedValue addr,
205+
const TypeLowering &lowering);
202206

203207
ManagedValue createFunctionArgument(SILType type, ValueDecl *decl);
204208

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ ManagedValue SILGenFunction::emitManagedRetain(SILLocation loc,
5757
assert(lowering.getLoweredType() == v->getType());
5858
if (lowering.isTrivial())
5959
return ManagedValue::forUnmanaged(v);
60+
if (v->getType().isObject() &&
61+
v.getOwnershipKind() == ValueOwnershipKind::Trivial)
62+
return ManagedValue::forUnmanaged(v);
6063
assert(!lowering.isAddressOnly() && "cannot retain an unloadable type");
6164

6265
v = lowering.emitCopyValue(B, loc, v);
@@ -74,6 +77,8 @@ ManagedValue SILGenFunction::emitManagedLoadCopy(SILLocation loc, SILValue v,
7477
v = lowering.emitLoadOfCopy(B, loc, v, IsNotTake);
7578
if (lowering.isTrivial())
7679
return ManagedValue::forUnmanaged(v);
80+
if (v.getOwnershipKind() == ValueOwnershipKind::Trivial)
81+
return ManagedValue::forUnmanaged(v);
7782
assert(!lowering.isAddressOnly() && "cannot retain an unloadable type");
7883
return emitManagedRValueWithCleanup(v, lowering);
7984
}
@@ -107,7 +112,8 @@ ManagedValue SILGenFunction::emitManagedStoreBorrow(SILLocation loc, SILValue v,
107112
ManagedValue SILGenFunction::emitManagedStoreBorrow(
108113
SILLocation loc, SILValue v, SILValue addr, const TypeLowering &lowering) {
109114
assert(lowering.getLoweredType().getObjectType() == v->getType());
110-
if (lowering.isTrivial()) {
115+
if (lowering.isTrivial() ||
116+
v.getOwnershipKind() == ValueOwnershipKind::Trivial) {
111117
lowering.emitStore(B, loc, v, addr, StoreOwnershipQualifier::Trivial);
112118
return ManagedValue::forUnmanaged(v);
113119
}
@@ -118,8 +124,6 @@ ManagedValue SILGenFunction::emitManagedStoreBorrow(
118124

119125
ManagedValue SILGenFunction::emitManagedBeginBorrow(SILLocation loc,
120126
SILValue v) {
121-
if (v.getOwnershipKind() == ValueOwnershipKind::Guaranteed)
122-
return ManagedValue::forUnmanaged(v);
123127
auto &lowering = F.getTypeLowering(v->getType());
124128
return emitManagedBeginBorrow(loc, v, lowering);
125129
}
@@ -131,8 +135,13 @@ SILGenFunction::emitManagedBeginBorrow(SILLocation loc, SILValue v,
131135
v->getType().getObjectType());
132136
if (lowering.isTrivial())
133137
return ManagedValue::forUnmanaged(v);
138+
139+
if (v.getOwnershipKind() == ValueOwnershipKind::Trivial)
140+
return ManagedValue::forUnmanaged(v);
141+
134142
if (v.getOwnershipKind() == ValueOwnershipKind::Guaranteed)
135143
return ManagedValue::forUnmanaged(v);
144+
136145
auto *bbi = B.createBeginBorrow(loc, v);
137146
return emitManagedBorrowedRValueWithCleanup(v, bbi, lowering);
138147
}
@@ -263,9 +272,14 @@ ManagedValue SILGenFunction::emitManagedBorrowedRValueWithCleanup(
263272
if (lowering.isTrivial())
264273
return ManagedValue::forUnmanaged(borrowed);
265274

275+
if (original->getType().isObject() &&
276+
original.getOwnershipKind() == ValueOwnershipKind::Trivial)
277+
return ManagedValue::forUnmanaged(borrowed);
278+
266279
if (borrowed->getType().isObject()) {
267280
Cleanups.pushCleanup<EndBorrowCleanup>(original, borrowed);
268281
}
282+
269283
return ManagedValue(borrowed, CleanupHandle::invalid());
270284
}
271285

@@ -279,7 +293,10 @@ ManagedValue SILGenFunction::emitManagedRValueWithCleanup(SILValue v,
279293
assert(lowering.getLoweredType() == v->getType());
280294
if (lowering.isTrivial())
281295
return ManagedValue::forUnmanaged(v);
282-
296+
if (v->getType().isObject() &&
297+
v.getOwnershipKind() == ValueOwnershipKind::Trivial) {
298+
return ManagedValue::forUnmanaged(v);
299+
}
283300
return ManagedValue(v, enterDestroyCleanup(v));
284301
}
285302

@@ -1670,8 +1687,14 @@ ManagedValue SILGenFunction::getManagedValue(SILLocation loc,
16701687
if (valueTL.isTrivial())
16711688
return ManagedValue::forUnmanaged(value.getValue());
16721689

1673-
// If it's an object, retain and enter a release cleanup.
1690+
// If it's an object...
16741691
if (valueTy.isObject()) {
1692+
// See if we have more accurate information from the ownership kind. This
1693+
// detects trivial cases of enums.
1694+
if (value.getOwnershipKind() == ValueOwnershipKind::Trivial)
1695+
return ManagedValue::forUnmanaged(value.getValue());
1696+
1697+
// Otherwise, retain and enter a release cleanup.
16751698
valueTL.emitCopyValue(B, loc, value.getValue());
16761699
return emitManagedRValueWithCleanup(value.getValue(), valueTL);
16771700
}

‎test/SIL/ownership-verifier/opaque_use_verifier.sil

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %target-sil-opt -enable-sil-opaque-values -enable-sil-ownership -enable-sil-verify-all=0 -module-name Swift -o /dev/null 2>&1 %s
2+
// REQUIRES: asserts
3+
4+
// This file is meant to contain tests that previously the verifier treated
5+
// incorrectly. This is important to ensure that the verifier does not
6+
// regress. It should only deal with use matching of opaque types.
7+
8+
sil_stage canonical
9+
10+
import Builtin
11+
12+
sil @unconditional_checked_cast_opaque_test : $@convention(thin) <T> (Builtin.Int32) -> @out T {
13+
bb0(%0 : @trivial $Builtin.Int32):
14+
%1 = unconditional_checked_cast_opaque %0 : $Builtin.Int32 to $T
15+
return %1 : $T
16+
}

‎test/SILGen/builtins.swift

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -795,9 +795,13 @@ func refcast_any_punknown(_ o: AnyObject) -> PUnknown {
795795
// CHECK: [[BORROWED_P:%.*]] = begin_borrow [[P]]
796796
// CHECK: [[P_COPY:%.*]] = copy_value [[BORROWED_P]]
797797
// CHECK: [[T:%.*]] = builtin "unsafeGuaranteed"<A>([[P_COPY]] : $A)
798-
// CHECK: [[R:%.*]] = tuple_extract [[T]] : $(A, Builtin.Int8), 0
799-
// CHECK: [[K:%.*]] = tuple_extract [[T]] : $(A, Builtin.Int8), 1
800-
// CHECK: destroy_value [[R]] : $A
798+
// CHECK: [[BORROWED_T:%.*]] = begin_borrow [[T]]
799+
// CHECK: [[R:%.*]] = tuple_extract [[BORROWED_T]] : $(A, Builtin.Int8), 0
800+
// CHECK: [[COPY_R:%.*]] = copy_value [[R]]
801+
// CHECK: [[K:%.*]] = tuple_extract [[BORROWED_T]] : $(A, Builtin.Int8), 1
802+
// CHECK: destroy_value [[COPY_R]] : $A
803+
// CHECK: end_borrow [[BORROWED_T]] from [[T]]
804+
// CHECK: destroy_value [[T]]
801805
// CHECK: end_borrow [[BORROWED_P]] from [[P]]
802806
// CHECK: [[BORROWED_P:%.*]] = begin_borrow [[P]]
803807
// CHECK: [[P_COPY:%.*]] = copy_value [[BORROWED_P]]
@@ -817,9 +821,13 @@ func unsafeGuaranteed_class(_ a: A) -> A {
817821
// CHECK: [[BORROWED_P:%.*]] = begin_borrow [[P]]
818822
// CHECK: [[P_COPY:%.*]] = copy_value [[BORROWED_P]]
819823
// CHECK: [[T:%.*]] = builtin "unsafeGuaranteed"<T>([[P_COPY]] : $T)
820-
// CHECK: [[R:%.*]] = tuple_extract [[T]] : $(T, Builtin.Int8), 0
821-
// CHECK: [[K:%.*]] = tuple_extract [[T]] : $(T, Builtin.Int8), 1
822-
// CHECK: destroy_value [[R]] : $T
824+
// CHECK: [[BORROWED_T:%.*]] = begin_borrow [[T]]
825+
// CHECK: [[R:%.*]] = tuple_extract [[BORROWED_T]] : $(T, Builtin.Int8), 0
826+
// CHECK: [[COPY_R:%.*]] = copy_value [[R]]
827+
// CHECK: [[K:%.*]] = tuple_extract [[BORROWED_T]] : $(T, Builtin.Int8), 1
828+
// CHECK: destroy_value [[COPY_R]] : $T
829+
// CHECK: end_borrow [[BORROWED_T]] from [[T]]
830+
// CHECK: destroy_value [[T]]
823831
// CHECK: end_borrow [[BORROWED_P]] from [[P]]
824832
// CHECK: [[BORROWED_P:%.*]] = begin_borrow [[P]]
825833
// CHECK: [[P_RETURN:%.*]] = copy_value [[BORROWED_P]]
@@ -837,11 +845,15 @@ func unsafeGuaranteed_generic<T: AnyObject> (_ a: T) -> T {
837845
// CHECK: [[BORROWED_P:%.*]] = begin_borrow [[P]]
838846
// CHECK: [[P_COPY:%.*]] = copy_value [[BORROWED_P]]
839847
// CHECK: [[T:%.*]] = builtin "unsafeGuaranteed"<T>([[P_COPY]] : $T)
840-
// CHECK: [[R]] = tuple_extract [[T]] : $(T, Builtin.Int8), 0
841-
// CHECK: [[K]] = tuple_extract [[T]] : $(T, Builtin.Int8), 1
848+
// CHECK: [[BORROWED_T:%.*]] = begin_borrow [[T]]
849+
// CHECK: [[R]] = tuple_extract [[BORROWED_T]] : $(T, Builtin.Int8), 0
850+
// CHECK: [[COPY_R:%.*]] = copy_value [[R]]
851+
// CHECK: [[K]] = tuple_extract [[BORROWED_T]] : $(T, Builtin.Int8), 1
852+
// CHECK: end_borrow [[BORROWED_T]] from [[T]]
853+
// CHECK: destroy_value [[T]]
842854
// CHECK: end_borrow [[BORROWED_P]] from [[P]]
843855
// CHECK: destroy_value [[P]]
844-
// CHECK: [[S:%.*]] = tuple ([[R]] : $T, [[K]] : $Builtin.Int8)
856+
// CHECK: [[S:%.*]] = tuple ([[COPY_R]] : $T, [[K]] : $Builtin.Int8)
845857
// CHECK: return [[S]] : $(T, Builtin.Int8)
846858
// CHECK: }
847859
func unsafeGuaranteed_generic_return<T: AnyObject> (_ a: T) -> (T, Builtin.Int8) {

0 commit comments

Comments
 (0)