Skip to content

Commit c9edaee

Browse files
committed
[move-only] Make sure that we handle load [take] that are actually borrows correctly.
Before the previous commit, we didn't see load [take] very often since it occurs mostly on temporaries where we treat the copy_addr as the relevant take. Now that we are checking temporaries though, we need to support this behavior.
1 parent 5d9ab63 commit c9edaee

File tree

3 files changed

+82
-22
lines changed

3 files changed

+82
-22
lines changed

lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2523,29 +2523,27 @@ void MoveOnlyAddressCheckerPImpl::rewriteUses(
25232523
// scope, we would have emitted the scope expansion error during diagnostics.
25242524
for (auto pair : addressUseState.borrows) {
25252525
if (auto *li = dyn_cast<LoadInst>(pair.first)) {
2526-
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
2527-
// If we had a load [copy], borrow then we know that all of its destroys
2528-
// must have been destroy_value. So we can just gather up those
2529-
// destroy_value and use then to create a new load_borrow scope.
2530-
SILBuilderWithScope builder(li);
2531-
auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
2532-
// We use this auxillary list to avoid iterator invalidation of
2533-
// li->getConsumingUse();
2534-
StackList<DestroyValueInst *> toDelete(lbi->getFunction());
2535-
for (auto *consumeUse : li->getConsumingUses()) {
2536-
auto *dvi = cast<DestroyValueInst>(consumeUse->getUser());
2537-
SILBuilderWithScope destroyBuilder(dvi);
2538-
destroyBuilder.createEndBorrow(dvi->getLoc(), lbi);
2539-
toDelete.push_back(dvi);
2540-
changed = true;
2541-
}
2542-
while (!toDelete.empty())
2543-
toDelete.pop_back_val()->eraseFromParent();
2544-
2545-
li->replaceAllUsesWith(lbi);
2546-
li->eraseFromParent();
2547-
continue;
2526+
// If we had a load -> load_borrow then we know that all of its destroys
2527+
// must have been destroy_value. So we can just gather up those
2528+
// destroy_value and use then to create a new load_borrow scope.
2529+
SILBuilderWithScope builder(li);
2530+
auto *lbi = builder.createLoadBorrow(li->getLoc(), li->getOperand());
2531+
// We use this auxillary list to avoid iterator invalidation of
2532+
// li->getConsumingUse();
2533+
StackList<DestroyValueInst *> toDelete(lbi->getFunction());
2534+
for (auto *consumeUse : li->getConsumingUses()) {
2535+
auto *dvi = cast<DestroyValueInst>(consumeUse->getUser());
2536+
SILBuilderWithScope destroyBuilder(dvi);
2537+
destroyBuilder.createEndBorrow(dvi->getLoc(), lbi);
2538+
toDelete.push_back(dvi);
2539+
changed = true;
25482540
}
2541+
while (!toDelete.empty())
2542+
toDelete.pop_back_val()->eraseFromParent();
2543+
2544+
li->replaceAllUsesWith(lbi);
2545+
li->eraseFromParent();
2546+
continue;
25492547
}
25502548

25512549
llvm::dbgs() << "Borrow: " << *pair.first;

test/SILOptimizer/moveonly_addresschecker.sil

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ public struct NonTrivialStruct {
2929
sil @useNonTrivialStruct : $@convention(thin) (@guaranteed NonTrivialStruct) -> ()
3030
sil @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
3131
sil @consumeNonTrivialStructAddr : $@convention(thin) (@in NonTrivialStruct) -> ()
32+
sil @consumeNonTrivialStruct : $@convention(thin) (@owned NonTrivialStruct) -> ()
3233

3334
@_moveOnly
3435
public struct NonTrivialStructPair {
@@ -535,3 +536,42 @@ sil [ossa] @test_in_use : $@convention(thin) () -> () {
535536
%9999 = tuple ()
536537
return %9999 : $()
537538
}
539+
540+
//////////////////////////////////////////
541+
// MARK: Load Take As Borrow/Copy Tests //
542+
//////////////////////////////////////////
543+
544+
// CHECK-LABEL: sil [ossa] @load_take_as_load_borrow : $@convention(thin) (@owned NonTrivialStruct) -> () {
545+
// CHECK: [[ALLOC:%.*]] = alloc_stack $NonTrivialStruct
546+
// CHECK: [[BORROW:%.*]] = load_borrow [[ALLOC]]
547+
// CHECK: end_borrow [[BORROW]]
548+
// CHECK: } // end sil function 'load_take_as_load_borrow'
549+
sil [ossa] @load_take_as_load_borrow : $@convention(thin) (@owned NonTrivialStruct) -> () {
550+
bb0(%0 : @owned $NonTrivialStruct):
551+
%1 = alloc_stack $NonTrivialStruct
552+
%1a = mark_must_check [consumable_and_assignable] %1 : $*NonTrivialStruct
553+
store %0 to [init] %1a : $*NonTrivialStruct
554+
%2 = load [take] %1a : $*NonTrivialStruct
555+
destroy_value %2 : $NonTrivialStruct
556+
dealloc_stack %1 : $*NonTrivialStruct
557+
%9999 = tuple()
558+
return %9999 : $()
559+
}
560+
561+
// CHECK-LABEL: sil [ossa] @load_take_as_load_take : $@convention(thin) (@owned NonTrivialStruct) -> () {
562+
// CHECK: [[ALLOC:%.*]] = alloc_stack $NonTrivialStruct
563+
// CHECK: [[LOAD:%.*]] = load [take] [[ALLOC]]
564+
// CHECK: apply {{%.*}}([[LOAD]])
565+
// CHECK: } // end sil function 'load_take_as_load_take'
566+
sil [ossa] @load_take_as_load_take : $@convention(thin) (@owned NonTrivialStruct) -> () {
567+
bb0(%0 : @owned $NonTrivialStruct):
568+
%1 = alloc_stack $NonTrivialStruct
569+
%1a = mark_must_check [consumable_and_assignable] %1 : $*NonTrivialStruct
570+
store %0 to [init] %1a : $*NonTrivialStruct
571+
%2 = load [take] %1a : $*NonTrivialStruct
572+
%f = function_ref @consumeNonTrivialStruct : $@convention(thin) (@owned NonTrivialStruct) -> ()
573+
apply %f(%2) : $@convention(thin) (@owned NonTrivialStruct) -> ()
574+
dealloc_stack %1 : $*NonTrivialStruct
575+
%9999 = tuple()
576+
return %9999 : $()
577+
}

test/SILOptimizer/moveonly_addresschecker_diagnostics.sil

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,3 +507,25 @@ bb3:
507507
%23 = tuple ()
508508
return %23 : $()
509509
}
510+
511+
/////////////////////////////////////////////
512+
// MARK: Load Take as Load Copy Diagnostic //
513+
/////////////////////////////////////////////
514+
515+
sil [ossa] @load_take_as_load_copy : $@convention(thin) (@owned NonTrivialStruct) -> () {
516+
bb0(%0 : @owned $NonTrivialStruct):
517+
%1 = alloc_stack $NonTrivialStruct, let, name "foo"
518+
%1a = mark_must_check [consumable_and_assignable] %1 : $*NonTrivialStruct
519+
// expected-error @-1 {{'foo' consumed more than once}}
520+
store %0 to [init] %1a : $*NonTrivialStruct
521+
%2 = load [take] %1a : $*NonTrivialStruct
522+
%2a = copy_value %2 : $NonTrivialStruct
523+
%f = function_ref @consumingUseNonTrivialStruct : $@convention(thin) (@owned NonTrivialStruct) -> ()
524+
apply %f(%2) : $@convention(thin) (@owned NonTrivialStruct) -> ()
525+
// expected-note @-1 {{consumed here}}
526+
apply %f(%2a) : $@convention(thin) (@owned NonTrivialStruct) -> ()
527+
// expected-note @-1 {{consumed again here}}
528+
dealloc_stack %1 : $*NonTrivialStruct
529+
%9999 = tuple()
530+
return %9999 : $()
531+
}

0 commit comments

Comments
 (0)