Skip to content

Commit 4920927

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. (cherry picked from commit c9edaee)
1 parent f17da4c commit 4920927

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

25532551
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)