Skip to content

Commit 36d333c

Browse files
gottesmmgottesmm
authored
Merge pull request #68024 from gottesmm/pr-41b9d05e5b76061c0db11b5507cf80af25fc6dbe
[silgen] Hide ManagedValue::forUnmanaged and rewrite all uses to use more specific APIs
2 parents ec485ba + 8cfb3d2 commit 36d333c

26 files changed

+416
-234
lines changed

‎include/swift/SIL/SILBuilder.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,13 @@ class SILBuilder {
551551
PartialApplyInst::OnStackKind OnStack =
552552
PartialApplyInst::OnStackKind::NotOnStack,
553553
const GenericSpecializationInformation *SpecializationInfo = nullptr) {
554+
assert(OnStack == PartialApplyInst::OnStackKind::OnStack ||
555+
llvm::all_of(Args,
556+
[](SILValue value) {
557+
return value->getOwnershipKind().isCompatibleWith(
558+
OwnershipKind::Owned);
559+
}) &&
560+
"Must have an owned compatible object");
554561
return insert(PartialApplyInst::create(
555562
getSILDebugLocation(Loc), Fn, Args, Subs, CalleeConvention, *F,
556563
SpecializationInfo, OnStack));
@@ -900,6 +907,8 @@ class SILBuilder {
900907
EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
901908
assert(!SILArgument::isTerminatorResult(borrowedValue) &&
902909
"terminator results do not have end_borrow");
910+
assert(!isa<SILFunctionArgument>(borrowedValue) &&
911+
"Function arguments should never have an end_borrow");
903912
return insert(new (getModule())
904913
EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
905914
}

‎lib/SILGen/Cleanup.cpp

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,9 @@ ManagedValue CleanupCloner::clone(SILValue value) const {
401401
}
402402

403403
if (!hasCleanup) {
404-
return ManagedValue::forUnmanaged(value);
404+
if (value->getOwnershipKind().isCompatibleWith(OwnershipKind::Owned))
405+
return ManagedValue::forUnmanagedOwnedValue(value);
406+
return ManagedValue::forBorrowedRValue(value);
405407
}
406408

407409
if (writebackBuffer.has_value()) {
@@ -436,19 +438,19 @@ CleanupCloner::cloneForTuplePackExpansionComponent(SILValue tupleAddr,
436438
}
437439

438440
if (!hasCleanup) {
439-
return ManagedValue::forUnmanaged(tupleAddr);
441+
return ManagedValue::forBorrowedAddressRValue(tupleAddr);
440442
}
441443

442444
assert(!writebackBuffer.has_value());
443445
auto expansionTy = tupleAddr->getType().getTupleElementType(componentIndex);
444446
if (expansionTy.getPackExpansionPatternType().isTrivial(SGF.F))
445-
return ManagedValue::forUnmanaged(tupleAddr);
447+
return ManagedValue::forTrivialAddressRValue(tupleAddr);
446448

447449
auto cleanup =
448450
SGF.enterPartialDestroyRemainingTupleCleanup(tupleAddr, inducedPackType,
449451
componentIndex,
450452
/*start at */ SILValue());
451-
return ManagedValue(tupleAddr, cleanup);
453+
return ManagedValue::forOwnedAddressRValue(tupleAddr, cleanup);
452454
}
453455

454456
ManagedValue
@@ -460,19 +462,19 @@ CleanupCloner::cloneForPackPackExpansionComponent(SILValue packAddr,
460462
}
461463

462464
if (!hasCleanup) {
463-
return ManagedValue::forUnmanaged(packAddr);
465+
return ManagedValue::forBorrowedAddressRValue(packAddr);
464466
}
465467

466468
assert(!writebackBuffer.has_value());
467469
auto expansionTy = packAddr->getType().getPackElementType(componentIndex);
468470
if (expansionTy.getPackExpansionPatternType().isTrivial(SGF.F))
469-
return ManagedValue::forUnmanaged(packAddr);
471+
return ManagedValue::forTrivialAddressRValue(packAddr);
470472

471473
auto cleanup =
472474
SGF.enterPartialDestroyRemainingPackCleanup(packAddr, formalPackType,
473475
componentIndex,
474476
/*start at */ SILValue());
475-
return ManagedValue(packAddr, cleanup);
477+
return ManagedValue::forOwnedAddressRValue(packAddr, cleanup);
476478
}
477479

478480
ManagedValue
@@ -484,7 +486,7 @@ CleanupCloner::cloneForRemainingPackComponents(SILValue packAddr,
484486
}
485487

486488
if (!hasCleanup) {
487-
return ManagedValue::forUnmanaged(packAddr);
489+
return ManagedValue::forBorrowedAddressRValue(packAddr);
488490
}
489491

490492
assert(!writebackBuffer.has_value());
@@ -498,12 +500,12 @@ CleanupCloner::cloneForRemainingPackComponents(SILValue packAddr,
498500
}
499501

500502
if (isTrivial)
501-
return ManagedValue::forUnmanaged(packAddr);
503+
return ManagedValue::forTrivialAddressRValue(packAddr);
502504

503505
auto cleanup =
504506
SGF.enterDestroyRemainingPackComponentsCleanup(packAddr, formalPackType,
505507
firstComponentIndex);
506-
return ManagedValue(packAddr, cleanup);
508+
return ManagedValue::forOwnedAddressRValue(packAddr, cleanup);
507509
}
508510

509511
ManagedValue
@@ -515,7 +517,7 @@ CleanupCloner::cloneForRemainingTupleComponents(SILValue tupleAddr,
515517
}
516518

517519
if (!hasCleanup) {
518-
return ManagedValue::forUnmanaged(tupleAddr);
520+
return ManagedValue::forBorrowedAddressRValue(tupleAddr);
519521
}
520522

521523
assert(!writebackBuffer.has_value());
@@ -529,11 +531,11 @@ CleanupCloner::cloneForRemainingTupleComponents(SILValue tupleAddr,
529531
}
530532

531533
if (isTrivial)
532-
return ManagedValue::forUnmanaged(tupleAddr);
534+
return ManagedValue::forTrivialAddressRValue(tupleAddr);
533535

534536
auto cleanup =
535537
SGF.enterDestroyRemainingTupleElementsCleanup(tupleAddr,
536538
inducedPackType,
537539
firstComponentIndex);
538-
return ManagedValue(tupleAddr, cleanup);
540+
return ManagedValue::forOwnedAddressRValue(tupleAddr, cleanup);
539541
}

‎lib/SILGen/Initialization.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,8 @@ class TemporaryInitialization : public SingleBufferInitialization {
323323
CleanupHandle getInitializedCleanup() const { return Cleanup; }
324324

325325
ManagedValue getManagedAddress() const {
326-
return ManagedValue(getAddress(), getInitializedCleanup());
326+
return ManagedValue::forOwnedAddressRValue(getAddress(),
327+
getInitializedCleanup());
327328
}
328329
};
329330

‎lib/SILGen/ManagedValue.h

Lines changed: 66 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -66,50 +66,62 @@ class ManagedValue {
6666
: valueAndFlag(value, isLValue), cleanup(cleanup) {
6767
}
6868

69-
public:
70-
71-
ManagedValue() = default;
69+
/// Create a managed value for a +0 rvalue.
70+
///
71+
/// Please do not introduce new uses of this method! Instead use one of the
72+
/// static constructors below!
73+
static ManagedValue forUnmanaged(SILValue value) {
74+
assert(value && "No value specified");
75+
return ManagedValue(value, false, CleanupHandle::invalid());
76+
}
7277

7378
/// Create a managed value for a +1 rvalue.
7479
///
7580
/// Please do not introduce new uses of this method! Instead use one of the
7681
/// static constructors below.
77-
ManagedValue(SILValue value, CleanupHandle cleanup)
78-
: valueAndFlag(value, false), cleanup(cleanup) {
82+
explicit ManagedValue(SILValue value,
83+
CleanupHandle cleanup = CleanupHandle::invalid())
84+
: valueAndFlag(value, false), cleanup(cleanup) {
7985
assert(value && "No value specified?!");
8086
assert((!getType().isObject() ||
8187
value->getOwnershipKind() != OwnershipKind::None ||
8288
!hasCleanup()) &&
8389
"Objects with trivial ownership should never have a cleanup");
8490
}
8591

86-
/// Create a managed value for a +0 rvalue.
92+
public:
93+
ManagedValue() = default;
94+
95+
/// Sometimes SILGen wants to represent an owned value or owned address
96+
/// without a cleanup as a +0 value that must be copied to be consumed.
8797
///
88-
/// Please do not introduce new uses of this method! Instead use one of the
89-
/// static constructors below!
90-
static ManagedValue forUnmanaged(SILValue value) {
91-
assert(value && "No value specified");
92-
return ManagedValue(value, false, CleanupHandle::invalid());
98+
/// Please do not introduce new uses of this.
99+
///
100+
/// DISCUSSION: We purposely provide a specific API for code paths that use
101+
/// owned values (and assert the values are owned) so that users do not
102+
/// attempt to use this for borrowed values. All borrowed values need to use
103+
/// the borrowed value APIs.
104+
static ManagedValue forUnmanagedOwnedValue(SILValue value) {
105+
assert(value);
106+
assert(!value->getType().isObject() ||
107+
value->getOwnershipKind().isCompatibleWith(OwnershipKind::Owned));
108+
return ManagedValue(value);
109+
}
110+
111+
/// Wrap a value with OwnershipKind::Unowned in a ManagedValue. This must be
112+
/// copied before it is used.
113+
static ManagedValue forUnownedObjectValue(SILValue value) {
114+
assert(value);
115+
assert(value->getType().isObject());
116+
assert(value->getOwnershipKind().isCompatibleWith(OwnershipKind::Unowned));
117+
return ManagedValue(value);
93118
}
94119

95120
enum class ScopeKind {
96121
Lexical,
97122
FormalAccess,
98123
};
99124

100-
/// Given a value \p value, create a copy of it and return the relevant
101-
/// ManagedValue.
102-
static ManagedValue forCopyOwnedObjectRValue(SILGenFunction &SGF,
103-
SILLocation loc, SILValue value,
104-
ScopeKind kind) {
105-
assert(value && "No value specified");
106-
assert(value->getType().isObject());
107-
auto mv = ManagedValue::forUnmanaged(value);
108-
if (kind == ScopeKind::Lexical)
109-
return mv.copy(SGF, loc);
110-
return mv.formalAccessCopy(SGF, loc);
111-
}
112-
113125
/// Create a managed value for a SILValue whose ownership is
114126
/// forwarded. Creates a new cleanup for +1 values. Forwarded +0 values
115127
/// require no cleanup.
@@ -160,7 +172,10 @@ class ManagedValue {
160172
assert(value && "No value specified");
161173
assert(value->getType().isObject() &&
162174
"Expected borrowed rvalues to be objects");
163-
assert(value->getOwnershipKind() != OwnershipKind::None);
175+
if (value->getOwnershipKind() == OwnershipKind::None) {
176+
return forObjectRValueWithoutOwnership(value);
177+
}
178+
assert(value->getOwnershipKind() == OwnershipKind::Guaranteed);
164179
return ManagedValue(value, false, CleanupHandle::invalid());
165180
}
166181

@@ -169,6 +184,13 @@ class ManagedValue {
169184
forBorrowedAddressRValue(SILValue value) {
170185
assert(value && "No value specified");
171186
assert(value->getType().isAddress() && "Expected value to be an address");
187+
// We check for value->getFunction() here since we /could/ be passed
188+
// SILUndef here.
189+
if (auto *f = value->getFunction()) {
190+
if (value->getType().isTrivial(f)) {
191+
return forTrivialAddressRValue(value);
192+
}
193+
}
172194
assert(value->getOwnershipKind() == OwnershipKind::None &&
173195
"Addresses always have trivial ownership");
174196
return ManagedValue(value, false, CleanupHandle::invalid());
@@ -193,6 +215,16 @@ class ManagedValue {
193215
static ManagedValue forTrivialAddressRValue(SILValue value) {
194216
assert(value->getType().isAddress() && "Expected an address");
195217
assert(value->getOwnershipKind() == OwnershipKind::None);
218+
219+
// TODO: Add an assert that we have a trivial type here.
220+
//
221+
// DISCUSSION: We cannot do this today since we have problems along certain
222+
// materialization paths where we want to emit a borrow operation. To handle
223+
// those cases, we have loosened the rules of OSSA by allowing for store
224+
// [trivial] to take non-trivial .none parameters. This has hidden bugs
225+
// where SILGen emits a borrow to materialize a parameter for an @in
226+
// parameter. It just coincidently works since when we emit the
227+
// store_borrow, we use the store [trivial] instead. This should be fixed.
196228
return ManagedValue(value, false, CleanupHandle::invalid());
197229
}
198230

@@ -476,8 +508,8 @@ class ConsumableManagedValue {
476508
///
477509
/// You probably should not be using this; it's here to make it easy
478510
/// to find code that is probably wrong.
479-
ManagedValue asUnmanagedValue() const {
480-
return ManagedValue::forUnmanaged(Value.getValue());
511+
ManagedValue asUnmanagedOwnedValue() const {
512+
return ManagedValue::forUnmanagedOwnedValue(Value.getValue());
481513
}
482514

483515
/// Return the consumption rules appropriate for the final use of
@@ -489,15 +521,19 @@ class ConsumableManagedValue {
489521
ConsumableManagedValue asBorrowedOperand(SILGenFunction &SGF,
490522
SILLocation loc) const {
491523
if (getType().isAddress())
492-
return {asUnmanagedValue(), CastConsumptionKind::CopyOnSuccess};
493-
return {asUnmanagedValue().borrow(SGF, loc),
524+
return {asUnmanagedOwnedValue(), CastConsumptionKind::CopyOnSuccess};
525+
526+
if (Value.getOwnershipKind() == OwnershipKind::Guaranteed)
527+
return {Value, CastConsumptionKind::BorrowAlways};
528+
529+
return {asUnmanagedOwnedValue().borrow(SGF, loc),
494530
CastConsumptionKind::BorrowAlways};
495531
}
496532

497533
/// Return a managed value that's appropriate for copying this value and
498534
/// always consuming it.
499535
ConsumableManagedValue copy(SILGenFunction &SGF, SILLocation loc) const {
500-
return ConsumableManagedValue::forOwned(asUnmanagedValue().copy(SGF, loc));
536+
return ConsumableManagedValue::forOwned(Value.copy(SGF, loc));
501537
}
502538
};
503539

‎lib/SILGen/ResultPlan.cpp

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -810,11 +810,13 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
810810
if (handlerIsOptional) {
811811
block = SGF.B.createOptionalSome(loc, block, impTy);
812812
}
813-
813+
814814
// We don't need to manage the block because it's still on the stack. We
815815
// know we won't escape it locally so the callee can be responsible for
816816
// _Block_copy-ing it.
817-
return ManagedValue::forUnmanaged(block);
817+
//
818+
// InitBlockStorageHeader always has Unowned ownership.
819+
return ManagedValue::forUnownedObjectValue(block);
818820
}
819821

820822
void deferExecutorBreadcrumb(ExecutorBreadcrumb &&crumb) override {
@@ -899,9 +901,9 @@ class ForeignAsyncInitializationPlan final : public ResultPlan {
899901
SILValue(wrappedContinuation));
900902
SGF.emitApplyOfLibraryIntrinsic(
901903
loc, errorIntrinsic, subs,
902-
{continuationMV, ManagedValue::forCopyOwnedObjectRValue(
903-
SGF, loc, bridgedForeignError,
904-
ManagedValue::ScopeKind::Lexical)},
904+
{continuationMV,
905+
SGF.B.copyOwnedObjectRValue(loc, bridgedForeignError,
906+
ManagedValue::ScopeKind::Lexical)},
905907
SGFContext());
906908

907909
// Second, emit a branch from the end of the foreign error block to the

‎lib/SILGen/SILGenApply.cpp

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1360,12 +1360,13 @@ class SILGenApply : public Lowering::ExprVisitor<SILGenApply> {
13601360
// we installed. Install a new special cleanup.
13611361
if (superMV.getValue() != SGF.InitDelegationSelf.getValue()) {
13621362
SILValue underlyingSelf = SGF.InitDelegationSelf.getValue();
1363-
SGF.InitDelegationSelf = ManagedValue::forUnmanaged(underlyingSelf);
1363+
SGF.InitDelegationSelf =
1364+
ManagedValue::forUnmanagedOwnedValue(underlyingSelf);
13641365
CleanupHandle newWriteback = SGF.enterOwnedValueWritebackCleanup(
13651366
SGF.InitDelegationLoc.value(), SGF.InitDelegationSelfBox,
13661367
superMV.forward(SGF));
1367-
SGF.SuperInitDelegationSelf =
1368-
ManagedValue(superMV.getValue(), newWriteback);
1368+
SGF.SuperInitDelegationSelf = ManagedValue::forOwnedObjectRValue(
1369+
superMV.getValue(), newWriteback);
13691370
super = RValue(SGF, SGF.InitDelegationLoc.value(), superFormalType,
13701371
SGF.SuperInitDelegationSelf);
13711372
}
@@ -4328,7 +4329,7 @@ static void emitBorrowedLValueRecursive(SILGenFunction &SGF,
43284329
value = SGF.B.createFormalAccessLoadCopy(loc, value);
43294330
// Strip off the cleanup from the load [copy] since we do not want the
43304331
// cleanup to be forwarded.
4331-
value = ManagedValue::forUnmanaged(value.getValue());
4332+
value = ManagedValue::forUnmanagedOwnedValue(value.getValue());
43324333
} else {
43334334
value = SGF.B.createFormalAccessLoadBorrow(loc, value);
43344335
}
@@ -4507,7 +4508,7 @@ class BoxInitialization : public SingleBufferInitialization {
45074508
}
45084509

45094510
ManagedValue getManagedBox() const {
4510-
return ManagedValue(box, initCleanup);
4511+
return ManagedValue::forOwnedObjectRValue(box, initCleanup);
45114512
}
45124513
};
45134514

@@ -6224,7 +6225,7 @@ void SILGenFunction::emitUninitializedArrayDeallocation(SILLocation loc,
62246225
auto subMap = arrayTy->getContextSubstitutionMap(SGM.M.getSwiftModule(),
62256226
Ctx.getArrayDecl());
62266227
emitApplyOfLibraryIntrinsic(loc, deallocate, subMap,
6227-
ManagedValue::forUnmanaged(array),
6228+
ManagedValue::forUnmanagedOwnedValue(array),
62286229
SGFContext());
62296230
}
62306231

@@ -6247,9 +6248,9 @@ ManagedValue SILGenFunction::emitUninitializedArrayFinalization(SILLocation loc,
62476248
// Invoke the intrinsic.
62486249
auto subMap = arrayTy->getContextSubstitutionMap(SGM.M.getSwiftModule(),
62496250
Ctx.getArrayDecl());
6250-
RValue result = emitApplyOfLibraryIntrinsic(loc, finalize, subMap,
6251-
ManagedValue::forUnmanaged(arrayVal),
6252-
SGFContext());
6251+
RValue result = emitApplyOfLibraryIntrinsic(
6252+
loc, finalize, subMap, ManagedValue::forUnmanagedOwnedValue(arrayVal),
6253+
SGFContext());
62536254
return std::move(result).getScalarValue();
62546255
}
62556256

‎lib/SILGen/SILGenBackDeploy.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,9 @@ static void emitBackDeployForwardApplyAndReturnOrThrow(
143143

144144
// Emit error block.
145145
SGF.B.emitBlock(errorBB);
146-
SILValue error = errorBB->createPhiArgument(
147-
SGF.F.mapTypeIntoContext(fnConv.getSILErrorType(TEC)),
148-
OwnershipKind::Owned);
146+
ManagedValue error =
147+
SGF.B.createPhi(SGF.F.mapTypeIntoContext(fnConv.getSILErrorType(TEC)),
148+
OwnershipKind::Owned);
149149
SGF.B.createBranch(loc, SGF.ThrowDest.getBlock(), {error});
150150

151151
// Emit normal block.

0 commit comments

Comments
 (0)