Skip to content

Commit 643227e

Browse files
gottesmmgottesmm
authored
Merge pull request swiftlang#29600 from gottesmm/pr-d1b175b6bba114b57173be25ad701464ec91e4b9
[ownership] Change guaranteed args from transformation terminators to not require end_borrows.
2 parents bcb18ea + 6c5c853 commit 643227e

17 files changed

+200
-68
lines changed

‎include/swift/SIL/SILBuilder.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -802,6 +802,12 @@ class SILBuilder {
802802
}
803803

804804
EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
805+
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
806+
if (auto *ti = arg->getSingleTerminator()) {
807+
assert(!ti->isTransformationTerminator() &&
808+
"Transforming terminators do not have end_borrow");
809+
}
810+
}
805811
return insert(new (getModule())
806812
EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
807813
}

‎include/swift/SIL/SILInstruction.h

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7042,6 +7042,30 @@ class TermInst : public NonValueInstruction {
70427042
bool isProgramTerminating() const;
70437043

70447044
TermKind getTermKind() const { return TermKind(getKind()); }
7045+
7046+
/// Returns true if this is a transformation terminator.
7047+
bool isTransformationTerminator() const {
7048+
switch (getTermKind()) {
7049+
case TermKind::UnwindInst:
7050+
case TermKind::UnreachableInst:
7051+
case TermKind::ReturnInst:
7052+
case TermKind::ThrowInst:
7053+
case TermKind::YieldInst:
7054+
case TermKind::TryApplyInst:
7055+
case TermKind::BranchInst:
7056+
case TermKind::CondBranchInst:
7057+
case TermKind::SwitchValueInst:
7058+
case TermKind::SwitchEnumAddrInst:
7059+
case TermKind::DynamicMethodBranchInst:
7060+
case TermKind::CheckedCastAddrBranchInst:
7061+
case TermKind::CheckedCastValueBranchInst:
7062+
return false;
7063+
case TermKind::SwitchEnumInst:
7064+
case TermKind::CheckedCastBranchInst:
7065+
return true;
7066+
}
7067+
llvm_unreachable("Covered switch isn't covered.");
7068+
}
70457069
};
70467070

70477071
/// UnreachableInst - Position in the code which would be undefined to reach.

‎lib/SIL/DynamicCasts.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1254,9 +1254,7 @@ void swift::emitIndirectConditionalCastWithScalar(
12541254
case CastConsumptionKind::TakeOnSuccess:
12551255
break;
12561256
case CastConsumptionKind::CopyOnSuccess: {
1257-
SILValue originalSuccValue = succValue;
12581257
succValue = B.emitCopyValueOperation(loc, succValue);
1259-
B.emitEndBorrowOperation(loc, originalSuccValue);
12601258
B.emitEndBorrowOperation(loc, srcValue);
12611259
break;
12621260
}
@@ -1295,7 +1293,6 @@ void swift::emitIndirectConditionalCastWithScalar(
12951293
StoreOwnershipQualifier::Init);
12961294
break;
12971295
case CastConsumptionKind::CopyOnSuccess:
1298-
B.emitEndBorrowOperation(loc, failValue);
12991296
B.emitEndBorrowOperation(loc, srcValue);
13001297
break;
13011298
case CastConsumptionKind::BorrowAlways:

‎lib/SIL/OwnershipUtils.cpp

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,15 @@ bool swift::isGuaranteedForwardingValueKind(SILNodeKind kind) {
6666
}
6767

6868
bool swift::isGuaranteedForwardingValue(SILValue value) {
69+
// If we have an argument from a transforming terminator, we can forward
70+
// guaranteed.
71+
if (auto *arg = dyn_cast<SILArgument>(value)) {
72+
if (auto *ti = arg->getSingleTerminator()) {
73+
if (ti->isTransformationTerminator()) {
74+
return true;
75+
}
76+
}
77+
}
6978
return isGuaranteedForwardingValueKind(
7079
value->getKindOfRepresentativeSILNodeInObject());
7180
}
@@ -173,9 +182,18 @@ bool swift::getUnderlyingBorrowIntroducingValues(
173182
// Otherwise if v is an ownership forwarding value, add its defining
174183
// instruction
175184
if (isGuaranteedForwardingValue(v)) {
176-
auto *i = v->getDefiningInstruction();
177-
assert(i);
178-
llvm::transform(i->getAllOperands(), std::back_inserter(worklist),
185+
if (auto *i = v->getDefiningInstruction()) {
186+
llvm::transform(i->getAllOperands(), std::back_inserter(worklist),
187+
[](const Operand &op) -> SILValue { return op.get(); });
188+
continue;
189+
}
190+
191+
// Otherwise, we should have a block argument that is defined by a single
192+
// predecessor terminator.
193+
auto *arg = cast<SILPhiArgument>(v);
194+
auto *termInst = arg->getSingleTerminator();
195+
assert(termInst && termInst->isTransformationTerminator());
196+
llvm::transform(termInst->getAllOperands(), std::back_inserter(worklist),
179197
[](const Operand &op) -> SILValue { return op.get(); });
180198
continue;
181199
}

‎lib/SIL/SILOwnershipVerifier.cpp

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -334,9 +334,55 @@ bool SILValueOwnershipChecker::gatherUsers(
334334
continue;
335335
}
336336

337-
// Otherwise if we have a terminator, add any as uses any end_borrow to
338-
// ensure that the subscope is completely enclsed within the super scope. We
339-
// require all of our arguments to be either trivial or guaranteed.
337+
// See if our forwarding terminator is a transformation terminator. If so,
338+
// just add all of the uses of its successors to the worklist to visit as
339+
// users.
340+
if (ti->isTransformationTerminator()) {
341+
for (auto &succ : ti->getSuccessors()) {
342+
auto *succBlock = succ.getBB();
343+
344+
// If we do not have any arguments, then continue.
345+
if (succBlock->args_empty())
346+
continue;
347+
348+
// Otherwise, make sure that all arguments are trivial or guaranteed. If
349+
// we fail, emit an error.
350+
//
351+
// TODO: We could ignore this error and emit a more specific error on
352+
// the actual terminator.
353+
for (auto *succArg : succBlock->getSILPhiArguments()) {
354+
// *NOTE* We do not emit an error here since we want to allow for more
355+
// specific errors to be found during use_verification.
356+
//
357+
// TODO: Add a flag that associates the terminator instruction with
358+
// needing to be verified. If it isn't verified appropriately, assert
359+
// when the verifier is destroyed.
360+
auto succArgOwnershipKind = succArg->getOwnershipKind();
361+
if (!succArgOwnershipKind.isCompatibleWith(ownershipKind)) {
362+
// This is where the error would go.
363+
continue;
364+
}
365+
366+
// If we have an any value, just continue.
367+
if (succArgOwnershipKind == ValueOwnershipKind::None)
368+
continue;
369+
370+
// Otherwise add all users of this BBArg to the worklist to visit
371+
// recursively.
372+
llvm::copy(succArg->getUses(), std::back_inserter(users));
373+
}
374+
}
375+
continue;
376+
}
377+
378+
// Otherwise, we are dealing with the merging of true phis. To work with
379+
// this we will eventually need to create a notion of forwarding borrow
380+
// scopes.
381+
382+
// But until then, we validate that the argument has an end_borrow that acts
383+
// as a subscope that is compeltely enclosed within the scopes of all
384+
// incoming values. We require all of our arguments to be either trivial or
385+
// guaranteed.
340386
for (auto &succ : ti->getSuccessors()) {
341387
auto *succBlock = succ.getBB();
342388

‎lib/SILGen/SILGenBuilder.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,13 @@ ManagedValue SILGenBuilder::createGuaranteedPhiArgument(SILType type) {
199199
return SGF.emitManagedBorrowedArgumentWithCleanup(arg);
200200
}
201201

202+
ManagedValue
203+
SILGenBuilder::createGuaranteedTransformingTerminatorArgument(SILType type) {
204+
SILPhiArgument *arg =
205+
getInsertionBB()->createPhiArgument(type, ValueOwnershipKind::Guaranteed);
206+
return ManagedValue::forUnmanaged(arg);
207+
}
208+
202209
ManagedValue SILGenBuilder::createAllocRef(
203210
SILLocation loc, SILType refType, bool objc, bool canAllocOnStack,
204211
ArrayRef<SILType> inputElementTypes,

‎lib/SILGen/SILGenBuilder.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@ class SILGenBuilder : public SILBuilder {
127127
ManagedValue createOwnedPhiArgument(SILType type);
128128
ManagedValue createGuaranteedPhiArgument(SILType type);
129129

130+
/// For arguments from terminators that are "transforming terminators". These
131+
/// types of guaranteed arguments are validated as part of the operand of the
132+
/// transforming terminator since transforming terminators are guaranteed to
133+
/// be the only predecessor of our parent block.
134+
///
135+
/// NOTE: Two examples of transforming terminators are switch_enum,
136+
/// checked_cast_br.
137+
ManagedValue createGuaranteedTransformingTerminatorArgument(SILType type);
138+
130139
using SILBuilder::createMarkUninitialized;
131140
ManagedValue createMarkUninitialized(ValueDecl *decl, ManagedValue operand,
132141
MarkUninitializedInst::Kind muKind);

‎lib/SILGen/SILGenDynamicCast.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ namespace {
185185
// argument.
186186
ManagedValue argument;
187187
if (!shouldTakeOnSuccess(consumption)) {
188-
argument = SGF.B.createGuaranteedPhiArgument(
188+
argument = SGF.B.createGuaranteedTransformingTerminatorArgument(
189189
origTargetTL.getLoweredType());
190190
} else {
191191
argument =
@@ -236,7 +236,8 @@ namespace {
236236
switch (consumption) {
237237
case CastConsumptionKind::BorrowAlways:
238238
case CastConsumptionKind::CopyOnSuccess:
239-
SGF.B.createGuaranteedPhiArgument(operandValue.getType());
239+
SGF.B.createGuaranteedTransformingTerminatorArgument(
240+
operandValue.getType());
240241
handleFalse(None);
241242
break;
242243
case CastConsumptionKind::TakeAlways:

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,14 @@ namespace {
165165
struct EndBorrowCleanup : Cleanup {
166166
SILValue borrowedValue;
167167

168-
EndBorrowCleanup(SILValue borrowedValue)
169-
: borrowedValue(borrowedValue) {}
168+
EndBorrowCleanup(SILValue borrowedValue) : borrowedValue(borrowedValue) {
169+
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
170+
if (auto *ti = arg->getSingleTerminator()) {
171+
assert(!ti->isTransformationTerminator() &&
172+
"Transforming terminators do not have end_borrow");
173+
}
174+
}
175+
}
170176

171177
void emit(SILGenFunction &SGF, CleanupLocation l,
172178
ForUnwind_t forUnwind) override {

‎lib/SILGen/SILGenPattern.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1921,7 +1921,7 @@ void PatternMatchEmission::emitEnumElementObjectDispatch(
19211921

19221922
SILValue eltValue;
19231923
if (isPlusZero) {
1924-
origCMV = {SGF.B.createGuaranteedPhiArgument(eltTy),
1924+
origCMV = {SGF.B.createGuaranteedTransformingTerminatorArgument(eltTy),
19251925
CastConsumptionKind::BorrowAlways};
19261926
} else {
19271927
origCMV = {SGF.B.createOwnedPhiArgument(eltTy),
@@ -1971,7 +1971,7 @@ void PatternMatchEmission::emitEnumElementObjectDispatch(
19711971
if (SILBasicBlock *defaultBB = blocks.getDefaultBlock()) {
19721972
SGF.B.setInsertionPoint(defaultBB);
19731973
if (isPlusZero) {
1974-
SGF.B.createGuaranteedPhiArgument(src.getType());
1974+
SGF.B.createGuaranteedTransformingTerminatorArgument(src.getType());
19751975
} else {
19761976
SGF.B.createOwnedPhiArgument(src.getType());
19771977
}

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

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,6 @@ bb0(%0 : @owned $Optional<Builtin.NativeObject>):
235235
switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
236236

237237
bb1(%1 : @guaranteed $Builtin.NativeObject):
238-
end_borrow %1 : $Builtin.NativeObject
239238
br bb3
240239

241240
bb2:
@@ -251,7 +250,7 @@ bb3:
251250
// CHECK: Found use after free?!
252251
// CHECK: Value: %1 = begin_borrow %0 : $Optional<Builtin.NativeObject>
253252
// CHECK: Consuming User: end_borrow %1 : $Optional<Builtin.NativeObject>
254-
// CHECK: Non Consuming User: end_borrow %3 : $Builtin.NativeObject
253+
// CHECK: Non Consuming User: %5 = unchecked_ref_cast %3 : $Builtin.NativeObject to $Builtin.NativeObject
255254
// CHECK: Block: bb1
256255
sil [ossa] @switch_enum_guaranteed_arg_outlives_original_value : $@convention(thin) (@owned Optional<Builtin.NativeObject>) -> () {
257256
bb0(%0 : @owned $Optional<Builtin.NativeObject>):
@@ -260,7 +259,7 @@ bb0(%0 : @owned $Optional<Builtin.NativeObject>):
260259

261260
bb1(%2 : @guaranteed $Builtin.NativeObject):
262261
end_borrow %1 : $Optional<Builtin.NativeObject>
263-
end_borrow %2 : $Builtin.NativeObject
262+
%2a = unchecked_ref_cast %2 : $Builtin.NativeObject to $Builtin.NativeObject
264263
br bb3
265264

266265
bb2:
@@ -312,7 +311,6 @@ bb0(%0 : @guaranteed $Builtin.NativeObject):
312311
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2
313312

314313
bb1(%1 : @guaranteed $SuperKlass):
315-
end_borrow %1 : $SuperKlass
316314
br bb3
317315

318316
bb2(%2 : @owned $Builtin.NativeObject):
@@ -339,7 +337,6 @@ bb1(%1 : @owned $SuperKlass):
339337
br bb3
340338

341339
bb2(%2 : @guaranteed $Builtin.NativeObject):
342-
end_borrow %2 : $Builtin.NativeObject
343340
br bb3
344341

345342
bb3:
@@ -363,11 +360,9 @@ bb0(%0 : @owned $Builtin.NativeObject):
363360
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2
364361

365362
bb1(%1 : @guaranteed $SuperKlass):
366-
end_borrow %1 : $SuperKlass
367363
br bb3
368364

369365
bb2(%2 : @guaranteed $Builtin.NativeObject):
370-
end_borrow %2 : $Builtin.NativeObject
371366
br bb3
372367

373368
bb3:
@@ -387,7 +382,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
387382
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2
388383

389384
bb1(%1 : @guaranteed $SuperKlass):
390-
end_borrow %1 : $SuperKlass
391385
br bb3
392386

393387
bb2(%2 : @owned $Builtin.NativeObject):
@@ -414,7 +408,6 @@ bb1(%1 : @owned $SuperKlass):
414408
br bb3
415409

416410
bb2(%2 : @guaranteed $Builtin.NativeObject):
417-
end_borrow %2 : $Builtin.NativeObject
418411
br bb3
419412

420413
bb3:
@@ -426,21 +419,21 @@ bb3:
426419
// CHECK: Found use after free?!
427420
// CHECK: Value: %1 = begin_borrow %0 : $Builtin.NativeObject
428421
// CHECK: Consuming User: end_borrow %1 : $Builtin.NativeObject
429-
// CHECK: Non Consuming User: end_borrow %7 : $Builtin.NativeObject
422+
// CHECK: Non Consuming User: %9 = unchecked_ref_cast %7 : $Builtin.NativeObject to $Builtin.NativeObject
430423
// CHECK: Block: bb2
431424
sil [ossa] @checked_cast_br_guaranteed_arg_outlives_original_value : $@convention(thin) (@owned Builtin.NativeObject) -> () {
432425
bb0(%0 : @owned $Builtin.NativeObject):
433426
%1 = begin_borrow %0 : $Builtin.NativeObject
434427
checked_cast_br %1 : $Builtin.NativeObject to SuperKlass, bb1, bb2
435428

436429
bb1(%2 : @guaranteed $SuperKlass):
437-
end_borrow %2 : $SuperKlass
438430
end_borrow %1 : $Builtin.NativeObject
431+
%2a = unchecked_ref_cast %2 : $SuperKlass to $SuperKlass
439432
br bb3
440433

441434
bb2(%3 : @guaranteed $Builtin.NativeObject):
442435
end_borrow %1 : $Builtin.NativeObject
443-
end_borrow %3 : $Builtin.NativeObject
436+
%3a = unchecked_ref_cast %3 : $Builtin.NativeObject to $Builtin.NativeObject
444437
br bb3
445438

446439
bb3:

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,6 @@ bb0(%0 : @guaranteed $Builtin.NativeObject):
518518
switch_enum %1 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
519519

520520
bb1(%2 : @guaranteed $Builtin.NativeObject):
521-
end_borrow %2 : $Builtin.NativeObject
522521
br bb3
523522

524523
bb2:
@@ -536,7 +535,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
536535
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
537536

538537
bb1(%3 : @guaranteed $Builtin.NativeObject):
539-
end_borrow %3 : $Builtin.NativeObject
540538
end_borrow %1 : $Builtin.NativeObject
541539
br bb3
542540

@@ -557,7 +555,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
557555
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
558556

559557
bb1(%3 : @guaranteed $Builtin.NativeObject):
560-
end_borrow %3 : $Builtin.NativeObject
561558
br bb3
562559

563560
bb2:
@@ -577,7 +574,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
577574
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2
578575

579576
bb1(%3 : @guaranteed $Builtin.NativeObject):
580-
end_borrow %3 : $Builtin.NativeObject
581577
br bb3
582578

583579
bb2:
@@ -784,11 +780,9 @@ bb6:
784780
checked_cast_br %6 : $Builtin.NativeObject to SuperKlass, bb7, bb8
785781

786782
bb7(%7 : @guaranteed $SuperKlass):
787-
end_borrow %7 : $SuperKlass
788783
br bb9
789784

790785
bb8(%8 : @guaranteed $Builtin.NativeObject):
791-
end_borrow %8 : $Builtin.NativeObject
792786
br bb9
793787

794788
bb9:
@@ -1195,4 +1189,3 @@ bb0(%0 : @guaranteed $Builtin.NativeObject, %1 : @owned $Builtin.NativeObject):
11951189
%9999 = tuple()
11961190
return %9999 : $()
11971191
}
1198-

0 commit comments

Comments
 (0)