Skip to content

Commit 6c5c853

Browse files
gottesmmgottesmm
committed
[ownership] Change guaranteed args from transformation terminators to not require end_borrows.
For those who are unaware, a transformation terminator is a terminator like switch_enum/checked_cast_br that always dominate their successor blocks. Since they dominate their successor blocks by design and transform their input into the args form, we can validate that they obey guaranteed ownership semantics just like a forwarding instruction. Beyond removing unnecessary code bloat, this also makes it significantly more easier to optimize/work with transformation terminators when converting @owned -> @guaranteed since we do not need to find end_borrow points when the owned value is consumed. <rdar://problem/59097063>
1 parent 3c582f4 commit 6c5c853

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)