Skip to content

Commit 8f5afb6

Browse files
jckarterjckarter
committed
SIL: Treat store_borrow as borrowing its source, and have the move-only checker account for borrow scopes.
When rewriting uses of a noncopyable value, the move-only checker failed to take into account the scope of borrowing uses when establishing the final lifetimes of values. One way this manifested was when borrowed values get reabstracted from value to in-memory representations, using a store_borrow instruction, the lifetime of the original borrow would be ended immediately after the store_borrow begins rather than after the matching end_borrow. Fix this by, first, changing `store_borrow` to be treated as a borrowing use of its source rather than an interior-pointer use; this should be more accurate overall since `store_borrow` borrows the entire source value for a well-scoped duration balanced by `end_borrow` instructions. That done, change MoveOnlyBorrowToDestructureUtils so that when it sees a borrow use, it ends the borrow at the end(s) of the use's borrow scope, instead of immediately after the beginning of the use.
1 parent e2ff4a3 commit 8f5afb6

File tree

9 files changed

+150
-49
lines changed

9 files changed

+150
-49
lines changed

‎include/swift/SIL/OwnershipUtils.h

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,7 @@ class BorrowingOperandKind {
254254
enum Kind : uint8_t {
255255
Invalid = 0,
256256
BeginBorrow,
257+
StoreBorrow,
257258
BeginApply,
258259
Branch,
259260
Apply,
@@ -277,6 +278,8 @@ class BorrowingOperandKind {
277278
return Kind::Invalid;
278279
case SILInstructionKind::BeginBorrowInst:
279280
return Kind::BeginBorrow;
281+
case SILInstructionKind::StoreBorrowInst:
282+
return Kind::StoreBorrow;
280283
case SILInstructionKind::BeginApplyInst:
281284
return Kind::BeginApply;
282285
case SILInstructionKind::BranchInst:
@@ -386,6 +389,7 @@ struct BorrowingOperand {
386389
case BorrowingOperandKind::Invalid:
387390
llvm_unreachable("Using invalid case?!");
388391
case BorrowingOperandKind::BeginBorrow:
392+
case BorrowingOperandKind::StoreBorrow:
389393
case BorrowingOperandKind::BeginApply:
390394
case BorrowingOperandKind::Apply:
391395
case BorrowingOperandKind::TryApply:
@@ -418,6 +422,7 @@ struct BorrowingOperand {
418422
case BorrowingOperandKind::BeginBorrow:
419423
case BorrowingOperandKind::Branch:
420424
return true;
425+
case BorrowingOperandKind::StoreBorrow:
421426
case BorrowingOperandKind::BeginApply:
422427
case BorrowingOperandKind::Apply:
423428
case BorrowingOperandKind::TryApply:
@@ -790,7 +795,6 @@ class InteriorPointerOperandKind {
790795
RefTailAddr,
791796
OpenExistentialBox,
792797
ProjectBox,
793-
StoreBorrow,
794798
};
795799

796800
private:
@@ -819,8 +823,6 @@ class InteriorPointerOperandKind {
819823
return Kind::OpenExistentialBox;
820824
case SILInstructionKind::ProjectBoxInst:
821825
return Kind::ProjectBox;
822-
case SILInstructionKind::StoreBorrowInst:
823-
return Kind::StoreBorrow;
824826
}
825827
}
826828

@@ -839,8 +841,6 @@ class InteriorPointerOperandKind {
839841
return Kind::OpenExistentialBox;
840842
case ValueKind::ProjectBoxInst:
841843
return Kind::ProjectBox;
842-
case ValueKind::StoreBorrowInst:
843-
return Kind::StoreBorrow;
844844
}
845845
}
846846

@@ -897,8 +897,7 @@ struct InteriorPointerOperand {
897897
case InteriorPointerOperandKind::RefElementAddr:
898898
case InteriorPointerOperandKind::RefTailAddr:
899899
case InteriorPointerOperandKind::OpenExistentialBox:
900-
case InteriorPointerOperandKind::ProjectBox:
901-
case InteriorPointerOperandKind::StoreBorrow: {
900+
case InteriorPointerOperandKind::ProjectBox: {
902901
// Ok, we have a valid instruction. Return the relevant operand.
903902
auto *op =
904903
&cast<SingleValueInstruction>(resultValue)->getAllOperands()[0];
@@ -941,8 +940,6 @@ struct InteriorPointerOperand {
941940
return cast<OpenExistentialBoxInst>(operand->getUser());
942941
case InteriorPointerOperandKind::ProjectBox:
943942
return cast<ProjectBoxInst>(operand->getUser());
944-
case InteriorPointerOperandKind::StoreBorrow:
945-
return cast<StoreBorrowInst>(operand->getUser());
946943
}
947944
llvm_unreachable("Covered switch isn't covered?!");
948945
}

‎lib/SIL/IR/OperandOwnership.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ OperandOwnership OperandOwnershipClassifier::visitBranchInst(BranchInst *bi) {
452452
OperandOwnership
453453
OperandOwnershipClassifier::visitStoreBorrowInst(StoreBorrowInst *i) {
454454
if (getValue() == i->getSrc()) {
455-
return OperandOwnership::InteriorPointer;
455+
return OperandOwnership::Borrow;
456456
}
457457
return OperandOwnership::TrivialUse;
458458
}

‎lib/SIL/Utils/OwnershipUtils.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -600,6 +600,9 @@ void BorrowingOperandKind::print(llvm::raw_ostream &os) const {
600600
case Kind::BeginBorrow:
601601
os << "BeginBorrow";
602602
return;
603+
case Kind::StoreBorrow:
604+
os << "StoreBorrow";
605+
return;
603606
case Kind::BeginApply:
604607
os << "BeginApply";
605608
return;
@@ -649,6 +652,7 @@ bool BorrowingOperand::hasEmptyRequiredEndingUses() const {
649652
case BorrowingOperandKind::Invalid:
650653
llvm_unreachable("Using invalid case");
651654
case BorrowingOperandKind::BeginBorrow:
655+
case BorrowingOperandKind::StoreBorrow:
652656
case BorrowingOperandKind::BeginApply:
653657
case BorrowingOperandKind::BeginAsyncLet:
654658
case BorrowingOperandKind::PartialApplyStack: {
@@ -687,6 +691,20 @@ bool BorrowingOperand::visitScopeEndingUses(
687691
// false.
688692
return !deadBorrow;
689693
}
694+
case BorrowingOperandKind::StoreBorrow: {
695+
bool deadBorrow = true;
696+
for (auto *use : cast<StoreBorrowInst>(op->getUser())->getUses()) {
697+
if (isa<EndBorrowInst>(use->getUser())) {
698+
deadBorrow = false;
699+
if (!func(use))
700+
return false;
701+
}
702+
}
703+
// FIXME: special case for dead borrows. This is dangerous because clients
704+
// only expect visitScopeEndingUses to return false if the visitor returned
705+
// false.
706+
return !deadBorrow;
707+
}
690708
case BorrowingOperandKind::BeginApply: {
691709
bool deadApply = true;
692710
auto *user = cast<BeginApplyInst>(op->getUser());
@@ -763,6 +781,7 @@ BorrowedValue BorrowingOperand::getBorrowIntroducingUserResult() const {
763781
case BorrowingOperandKind::Yield:
764782
case BorrowingOperandKind::PartialApplyStack:
765783
case BorrowingOperandKind::BeginAsyncLet:
784+
case BorrowingOperandKind::StoreBorrow:
766785
return BorrowedValue();
767786

768787
case BorrowingOperandKind::BeginBorrow: {
@@ -792,6 +811,7 @@ SILValue BorrowingOperand::getScopeIntroducingUserResult() {
792811
case BorrowingOperandKind::BeginAsyncLet:
793812
case BorrowingOperandKind::PartialApplyStack:
794813
case BorrowingOperandKind::BeginBorrow:
814+
case BorrowingOperandKind::StoreBorrow:
795815
return cast<SingleValueInstruction>(op->getUser());
796816

797817
case BorrowingOperandKind::BeginApply:

‎lib/SILOptimizer/Mandatory/MoveOnlyBorrowToDestructureUtils.cpp

Lines changed: 74 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,17 @@ bool Implementation::gatherUses(SILValue value) {
437437
false /*is lifetime ending*/}});
438438
liveness.updateForUse(nextUse->getUser(), *leafRange,
439439
false /*is lifetime ending*/);
440+
// The liveness extends to the scope-ending uses of the borrow.
441+
BorrowingOperand(nextUse).visitScopeEndingUses([&](Operand *end) -> bool {
442+
if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
443+
return false;
444+
}
445+
LLVM_DEBUG(llvm::dbgs() << " ++ Scope-ending use: ";
446+
end->getUser()->print(llvm::dbgs()));
447+
liveness.updateForUse(end->getUser(), *leafRange,
448+
false /*is lifetime ending*/);
449+
return true;
450+
});
440451
instToInterestingOperandIndexMap.insert(nextUse->getUser(), nextUse);
441452

442453
continue;
@@ -1001,6 +1012,48 @@ dumpIntervalMap(IntervalMapAllocator::Map &map) {
10011012
}
10021013
#endif
10031014

1015+
// Helper to insert end_borrows after the end of a non-consuming use. If the
1016+
// use is momentary, one end_borrow is inserted after the use. If it is an
1017+
// interior pointer projection or nested borrow, then end_borrows are inserted
1018+
// after every scope-ending instruction for the use.
1019+
static void insertEndBorrowsForNonConsumingUse(Operand *op,
1020+
SILValue borrow) {
1021+
if (auto iOp = InteriorPointerOperand::get(op)) {
1022+
LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after interior pointer scope:\n"
1023+
" ";
1024+
op->getUser()->print(llvm::dbgs()));
1025+
iOp.visitBaseValueScopeEndingUses([&](Operand *endScope) -> bool {
1026+
auto *endScopeInst = endScope->getUser();
1027+
LLVM_DEBUG(llvm::dbgs() << " ";
1028+
endScopeInst->print(llvm::dbgs()));
1029+
SILBuilderWithScope endBuilder(endScopeInst);
1030+
endBuilder.createEndBorrow(getSafeLoc(endScopeInst), borrow);
1031+
return true;
1032+
});
1033+
} else if (auto bOp = BorrowingOperand(op)) {
1034+
LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after borrow scope:\n"
1035+
" ";
1036+
op->getUser()->print(llvm::dbgs()));
1037+
bOp.visitScopeEndingUses([&](Operand *endScope) -> bool {
1038+
auto *endScopeInst = endScope->getUser();
1039+
LLVM_DEBUG(llvm::dbgs() << " ";
1040+
endScopeInst->print(llvm::dbgs()));
1041+
auto afterScopeInst = endScopeInst->getNextInstruction();
1042+
SILBuilderWithScope endBuilder(afterScopeInst);
1043+
endBuilder.createEndBorrow(getSafeLoc(afterScopeInst),
1044+
borrow);
1045+
return true;
1046+
});
1047+
} else {
1048+
auto *nextInst = op->getUser()->getNextInstruction();
1049+
LLVM_DEBUG(llvm::dbgs() << " -- Ending borrow after momentary use at: ";
1050+
nextInst->print(llvm::dbgs()));
1051+
SILBuilderWithScope endBuilder(nextInst);
1052+
endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow);
1053+
}
1054+
1055+
}
1056+
10041057
void Implementation::rewriteUses(InstructionDeleter *deleter) {
10051058
blocksToUses.setFrozen();
10061059

@@ -1129,6 +1182,8 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) {
11291182
}
11301183

11311184
// Otherwise, we need to insert a borrow.
1185+
LLVM_DEBUG(llvm::dbgs() << " Inserting borrow for: ";
1186+
inst->print(llvm::dbgs()));
11321187
SILBuilderWithScope borrowBuilder(inst);
11331188
SILValue borrow =
11341189
borrowBuilder.createBeginBorrow(getSafeLoc(inst), first);
@@ -1138,21 +1193,10 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) {
11381193
borrowBuilder.createGuaranteedMoveOnlyWrapperToCopyableValue(
11391194
getSafeLoc(inst), innerValue);
11401195
}
1196+
1197+
insertEndBorrowsForNonConsumingUse(&operand, borrow);
11411198

1142-
if (auto op = InteriorPointerOperand::get(&operand)) {
1143-
op.visitBaseValueScopeEndingUses([&](Operand *endScope) -> bool {
1144-
auto *endScopeInst = endScope->getUser();
1145-
SILBuilderWithScope endBuilder(endScopeInst);
1146-
endBuilder.createEndBorrow(getSafeLoc(endScopeInst), borrow);
1147-
return true;
1148-
});
1149-
} else {
1150-
auto *nextInst = inst->getNextInstruction();
1151-
SILBuilderWithScope endBuilder(nextInst);
1152-
endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow);
1153-
}
1154-
1155-
// NOTE: This needs to be /after/the interior pointer operand usage
1199+
// NOTE: This needs to be /after/ the interior pointer operand usage
11561200
// above so that we can use the end scope of our interior pointer base
11571201
// value.
11581202
// NOTE: oldInst may be nullptr if our operand is a SILArgument
@@ -1245,9 +1289,14 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) {
12451289
}
12461290
}
12471291

1292+
insertEndBorrowsForNonConsumingUse(&operand, borrow);
1293+
/*
12481294
auto *nextInst = inst->getNextInstruction();
1295+
LLVM_DEBUG(llvm::dbgs() << "ending after momentary use\n";
1296+
inst->print(llvm::dbgs()));
12491297
SILBuilderWithScope endBuilder(nextInst);
12501298
endBuilder.createEndBorrow(getSafeLoc(nextInst), borrow);
1299+
*/
12511300
continue;
12521301
}
12531302

@@ -1333,6 +1382,13 @@ void Implementation::rewriteUses(InstructionDeleter *deleter) {
13331382
}
13341383

13351384
void Implementation::cleanup() {
1385+
LLVM_DEBUG(llvm::dbgs()
1386+
<< "Performing BorrowToDestructureTransform::cleanup()!\n");
1387+
SWIFT_DEFER {
1388+
LLVM_DEBUG(llvm::dbgs() << "Function after cleanup!\n";
1389+
getMarkedValue()->getFunction()->dump());
1390+
};
1391+
13361392
// Then add destroys for any destructure elements that we inserted that we did
13371393
// not actually completely consume.
13381394
auto *fn = getMarkedValue()->getFunction();
@@ -1793,11 +1849,15 @@ bool BorrowToDestructureTransform::transform() {
17931849
// Then clean up all of our borrows/copies/struct_extracts which no longer
17941850
// have any uses...
17951851
{
1852+
LLVM_DEBUG(llvm::dbgs() << "Deleting dead instructions!\n");
1853+
17961854
InstructionDeleter deleter;
17971855
while (!borrowWorklist.empty()) {
17981856
deleter.recursivelyForceDeleteUsersAndFixLifetimes(
17991857
borrowWorklist.pop_back_val());
18001858
}
1859+
LLVM_DEBUG(llvm::dbgs() << "Function after deletion!\n";
1860+
impl.getMarkedValue()->getFunction()->dump());
18011861
}
18021862

18031863
return true;

‎lib/SILOptimizer/SILCombiner/SILCombinerBuiltinVisitors.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,6 @@ SILCombiner::optimizeBuiltinCOWBufferForReadingOSSA(BuiltinInst *bi) {
162162
return;
163163
case InteriorPointerOperandKind::OpenExistentialBox:
164164
case InteriorPointerOperandKind::ProjectBox:
165-
case InteriorPointerOperandKind::StoreBorrow:
166165
// Can not mark this immutable.
167166
return;
168167
}

‎lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ static bool isDestroyOfCopyOf(SILInstruction *instruction, SILValue def) {
129129
//===----------------------------------------------------------------------===//
130130

131131
bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
132+
LLVM_DEBUG(llvm::dbgs() << "Computing canonical liveness from:\n";
133+
getCurrentDef()->print(llvm::dbgs()));
132134
defUseWorklist.initialize(getCurrentDef());
133135
// Only the first level of reborrows need to be consider. All nested inner
134136
// adjacent reborrows and phis are encapsulated within their lifetimes.
@@ -140,7 +142,13 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
140142
});
141143
}
142144
while (SILValue value = defUseWorklist.pop()) {
145+
LLVM_DEBUG(llvm::dbgs() << " Uses of value:\n";
146+
value->print(llvm::dbgs()));
147+
143148
for (Operand *use : value->getUses()) {
149+
LLVM_DEBUG(llvm::dbgs() << " Use:\n";
150+
use->getUser()->print(llvm::dbgs()));
151+
144152
auto *user = use->getUser();
145153
// Recurse through copies.
146154
if (auto *copy = dyn_cast<CopyValueInst>(user)) {
@@ -159,7 +167,7 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
159167
continue;
160168
}
161169
}
162-
switch (use->getOperandOwnership()) {
170+
switch (auto ownership = use->getOperandOwnership()) {
163171
case OperandOwnership::NonUse:
164172
break;
165173
case OperandOwnership::TrivialUse:
@@ -169,6 +177,7 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
169177
// escape. Is it legal to canonicalize ForwardingUnowned?
170178
case OperandOwnership::ForwardingUnowned:
171179
case OperandOwnership::PointerEscape:
180+
LLVM_DEBUG(llvm::dbgs() << " Value escaped! Giving up\n");
172181
return false;
173182
case OperandOwnership::InstantaneousUse:
174183
case OperandOwnership::UnownedInstantaneousUse:
@@ -197,7 +206,8 @@ bool CanonicalizeOSSALifetime::computeCanonicalLiveness() {
197206
break;
198207
case OperandOwnership::Borrow:
199208
if (liveness->updateForBorrowingOperand(use)
200-
!= InnerBorrowKind::Contained) {
209+
!= InnerBorrowKind::Contained) {
210+
LLVM_DEBUG(llvm::dbgs() << " Inner borrow can't be contained! Giving up\n");
201211
return false;
202212
}
203213
break;
@@ -1133,11 +1143,13 @@ void CanonicalizeOSSALifetime::rewriteCopies() {
11331143
//===----------------------------------------------------------------------===//
11341144

11351145
bool CanonicalizeOSSALifetime::computeLiveness() {
1136-
if (currentDef->getOwnershipKind() != OwnershipKind::Owned)
1137-
return false;
1138-
11391146
LLVM_DEBUG(llvm::dbgs() << " Canonicalizing: " << currentDef);
11401147

1148+
if (currentDef->getOwnershipKind() != OwnershipKind::Owned) {
1149+
LLVM_DEBUG(llvm::dbgs() << " not owned, never mind\n");
1150+
return false;
1151+
}
1152+
11411153
// Note: There is no need to register callbacks with this utility. 'onDelete'
11421154
// is the only one in use to handle dangling pointers, which could be done
11431155
// instead be registering a temporary handler with the pass. Canonicalization
@@ -1154,7 +1166,7 @@ bool CanonicalizeOSSALifetime::computeLiveness() {
11541166

11551167
// Step 1: compute liveness
11561168
if (!computeCanonicalLiveness()) {
1157-
LLVM_DEBUG(llvm::errs() << "Failed to compute canonical liveness?!\n");
1169+
LLVM_DEBUG(llvm::dbgs() << "Failed to compute canonical liveness?!\n");
11581170
clear();
11591171
return false;
11601172
}

0 commit comments

Comments
 (0)