Skip to content

Commit 5ca36e0

Browse files
committed
[TempRValueOpt] Fold new destroy_addrs into loads.
To avoid introducing new copies--which is illegal for move-only values-- don't rewrite `load [take]`s and `copy_addr [take]`s as `load [copy]`s and `copy_addr`s respectively and introduce new `destroy_addr`s after them. Instead, get the effect of folding such a newly created `destroy_addr` into the preceding rewritten `load [copy]` or `copy_addr`. Get that effect by neither modifying the `copy_addr [take]` or `load [take]` nor adding a subsequent `destroy_addr`. An example for each kind (`load [take]` and `copy_addr [take]`): ``` // Input 1 (`load [take]`) copy_addr [take] %src to [init] %tmp %val = load [take] %src // Old Output 1 %val = load [copy] %src destroy_addr %src // New Output 2 %val = load [take] %src ``` ``` // Input 2 (`copy_addr [take]`) copy_addr [take] %src to [init] %tmp copy_addr [take] %src to [init] %dst // Old Output 2 copy_addr %src to [init] %dst destroy_addr %src // New Output 2 copy_addr [take] %src to [init] %dst ``` rdar://107839979
1 parent 1fe148c commit 5ca36e0

File tree

2 files changed

+129
-14
lines changed

2 files changed

+129
-14
lines changed

lib/SILOptimizer/Transforms/TempRValueElimination.cpp

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -535,9 +535,10 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
535535
SILValue copySrc = copyInst->getSrc();
536536
assert(tempObj != copySrc && "can't initialize temporary with itself");
537537

538-
// If the source of the copyInst is taken, we must insert a compensating
539-
// destroy_addr. This must be done at the right spot: after the last use
540-
// tempObj, but before any (potential) re-initialization of the source.
538+
// If the source of the copyInst is taken, it must be deinitialized (via
539+
// destroy_addr, load [take], copy_addr [take]). This must be done at the
540+
// right spot: after the last use tempObj, but before any (potential)
541+
// re-initialization of the source.
541542
bool needFinalDeinit = copyInst->isTakeOfSrc();
542543

543544
// Scan all uses of the temporary storage (tempObj) to verify they all refer
@@ -601,7 +602,29 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
601602

602603
LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj);
603604

604-
if (needFinalDeinit) {
605+
// If copyInst's source must be deinitialized, whether that must be done via
606+
// a newly created destroy_addr.
607+
//
608+
// If lastLoadInst is a load or a copy_addr, then the deinitialization can be
609+
// done in that instruction.
610+
//
611+
// This is necessary for correctness: otherwise, copies of move-only values
612+
// would be introduced.
613+
bool needToInsertDestroy = [&]() {
614+
if (!needFinalDeinit)
615+
return false;
616+
if (lastLoadInst == copyInst)
617+
return true;
618+
if (auto *cai = dyn_cast<CopyAddrInst>(lastLoadInst)) {
619+
return cai->getSrc() != tempObj || !cai->isTakeOfSrc();
620+
}
621+
if (auto *li = dyn_cast<LoadInst>(lastLoadInst)) {
622+
return li->getOperand() != tempObj ||
623+
li->getOwnershipQualifier() != LoadOwnershipQualifier::Take;
624+
}
625+
return true;
626+
}();
627+
if (needToInsertDestroy) {
605628
// Compensate the [take] of the original copyInst.
606629
SILBuilderWithScope::insertAfter(lastLoadInst, [&] (SILBuilder &builder) {
607630
builder.createDestroyAddr(builder.getInsertionPoint()->getLoc(), copySrc);
@@ -630,16 +653,19 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) {
630653
auto *cai = cast<CopyAddrInst>(user);
631654
if (cai != copyInst) {
632655
assert(cai->getSrc() == tempObj);
633-
if (cai->isTakeOfSrc())
656+
if (cai->isTakeOfSrc() && (!needFinalDeinit || lastLoadInst != cai)) {
634657
cai->setIsTakeOfSrc(IsNotTake);
658+
}
635659
}
636660
use->set(copySrc);
637661
break;
638662
}
639663
case SILInstructionKind::LoadInst: {
640664
auto *li = cast<LoadInst>(user);
641-
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take)
665+
if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take &&
666+
(!needFinalDeinit || li != lastLoadInst)) {
642667
li->setOwnershipQualifier(LoadOwnershipQualifier::Copy);
668+
}
643669
use->set(copySrc);
644670
break;
645671
}

test/SILOptimizer/temp_rvalue_opt_ossa.sil

Lines changed: 97 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,17 @@ public enum FakeOptional<T> {
3939
case some(T)
4040
}
4141

42+
@_moveOnly struct MOS {}
4243

4344
sil [ossa] @getKlass : $@convention(thin) () -> @owned Klass
45+
sil [ossa] @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
46+
sil [ossa] @getMOS : $@convention(thin) () -> @owned MOS
4447

4548
sil @unknown : $@convention(thin) () -> ()
4649

4750
sil [ossa] @guaranteed_user : $@convention(thin) (@guaranteed Klass) -> ()
4851
sil [ossa] @guaranteed_user_with_result : $@convention(thin) (@guaranteed Klass) -> @out Klass
52+
sil [ossa] @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> ()
4953

5054
sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
5155
bb0(%0 : $*Klass):
@@ -767,8 +771,7 @@ bb0(%0 : $*Builtin.NativeObject):
767771
//
768772
// CHECK-LABEL: sil [ossa] @takeWithLoadRelease : $@convention(thin) (@in Builtin.NativeObject) -> () {
769773
// CHECK: bb0(%0 : $*Builtin.NativeObject):
770-
// CHECK: [[V:%.*]] = load [copy] %0 : $*Builtin.NativeObject
771-
// CHECK: destroy_addr %0 : $*Builtin.NativeObject
774+
// CHECK: [[V:%.*]] = load [take] %0 : $*Builtin.NativeObject
772775
// CHECK: apply %{{.*}}([[V]]) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
773776
// CHECK: destroy_value [[V]] : $Builtin.NativeObject
774777
// CHECK-LABEL: } // end sil function 'takeWithLoadRelease'
@@ -1516,9 +1519,7 @@ bb2:
15161519
// CHECK: [[ADDR:%[^,]+]] = alloc_stack $Klass
15171520
// CHECK: store [[INSTANCE_1]] to [init] [[ADDR]]
15181521
// CHECK: apply [[USER]]([[ADDR]])
1519-
// CHECK: [[INSTANCE_2:%[^,]+]] = load [copy] [[ADDR]]
1520-
// Extra copy: ^^^^^^^^^^^
1521-
// CHECK: destroy_addr [[ADDR]]
1522+
// CHECK: [[INSTANCE_2:%[^,]+]] = load [take] [[ADDR]]
15221523
// CHECK: dealloc_stack [[ADDR]]
15231524
// CHECK: return [[INSTANCE_2]]
15241525
// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_load_take'
@@ -1545,9 +1546,7 @@ sil [ossa] @take_from_original_copy_addr__final_use_load_take : $() -> @owned Kl
15451546
// CHECK: [[SRC:%[^,]+]] = alloc_stack $Klass
15461547
// CHECK: store [[INSTANCE_1]] to [init] [[SRC]]
15471548
// CHECK: apply [[USER]]([[SRC]])
1548-
// CHECK: copy_addr [[SRC]] to [init] [[OUT]]
1549-
// Extra copy: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1550-
// CHECK: destroy_addr [[SRC]]
1549+
// CHECK: copy_addr [take] [[SRC]] to [init] [[OUT]]
15511550
// CHECK: dealloc_stack [[SRC]]
15521551
// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_copy_addr_take'
15531552
sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_take : $() -> @out Klass {
@@ -1567,6 +1566,68 @@ entry(%out : $*Klass):
15671566
return %retval : $()
15681567
}
15691568

1569+
// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_field_load_copy : {{.*}} {
1570+
// CHECK: [[GET:%[^,]+]] = function_ref @getNonTrivialStruct
1571+
// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result_NTS
1572+
// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]()
1573+
// CHECK: [[SRC:%[^,]+]] = alloc_stack $NonTrivialStruct
1574+
// CHECK: store [[INSTANCE]] to [init] [[SRC]]
1575+
// CHECK: apply [[USER]]([[SRC]])
1576+
// CHECK: [[FIELD_ADDR:%[^,]+]] = struct_element_addr [[SRC]]
1577+
// CHECK: [[FIELD:%[^,]+]] = load [copy] [[FIELD_ADDR]]
1578+
// CHECK: destroy_addr [[SRC]]
1579+
// CHECK: dealloc_stack [[SRC]]
1580+
// CHECK: return [[FIELD]]
1581+
// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_field_load_copy'
1582+
sil [ossa] @take_from_original_copy_addr__final_use_field_load_copy : $() -> @owned Klass {
1583+
%get = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
1584+
%user = function_ref @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> ()
1585+
%instance_1 = apply %get() : $@convention(thin) () -> @owned NonTrivialStruct
1586+
%src = alloc_stack $NonTrivialStruct
1587+
store %instance_1 to [init] %src : $*NonTrivialStruct
1588+
apply %user(%src) : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> ()
1589+
%tmp = alloc_stack $NonTrivialStruct
1590+
copy_addr [take] %src to [init] %tmp : $*NonTrivialStruct
1591+
%field_addr = struct_element_addr %tmp : $*NonTrivialStruct, #NonTrivialStruct.val
1592+
%field = load [copy] %field_addr : $*Klass
1593+
destroy_addr %tmp : $*NonTrivialStruct
1594+
dealloc_stack %tmp : $*NonTrivialStruct
1595+
dealloc_stack %src : $*NonTrivialStruct
1596+
return %field : $Klass
1597+
}
1598+
1599+
// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_field_copy_addr_take : {{.*}} {
1600+
// CHECK: {{bb[0-9]+}}([[OUT:%[^,]+]] :
1601+
// CHECK: [[GET:%[^,]+]] = function_ref @getNonTrivialStruct
1602+
// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result_NTS
1603+
// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]()
1604+
// CHECK: [[SRC:%[^,]+]] = alloc_stack $NonTrivialStruct
1605+
// CHECK: store [[INSTANCE]] to [init] [[SRC]]
1606+
// CHECK: apply [[USER]]([[SRC]])
1607+
// CHECK: [[FIELD_ADDR:%[^,]+]] = struct_element_addr [[SRC]]
1608+
// CHECK: copy_addr [[FIELD_ADDR]] to [init] [[OUT]]
1609+
// CHECK: destroy_addr [[SRC]]
1610+
// CHECK: dealloc_stack [[SRC]]
1611+
// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_field_copy_addr_take'
1612+
sil [ossa] @take_from_original_copy_addr__final_use_field_copy_addr_take : $() -> @out Klass {
1613+
entry(%out : $*Klass):
1614+
%getNonTrivialStruct = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct
1615+
%user = function_ref @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> ()
1616+
%instance_1 = apply %getNonTrivialStruct() : $@convention(thin) () -> @owned NonTrivialStruct
1617+
%src = alloc_stack $NonTrivialStruct
1618+
store %instance_1 to [init] %src : $*NonTrivialStruct
1619+
apply %user(%src) : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> ()
1620+
%tmp = alloc_stack $NonTrivialStruct
1621+
copy_addr [take] %src to [init] %tmp : $*NonTrivialStruct
1622+
%field_addr = struct_element_addr %tmp : $*NonTrivialStruct, #NonTrivialStruct.val
1623+
copy_addr %field_addr to [init] %out : $*Klass
1624+
destroy_addr %tmp : $*NonTrivialStruct
1625+
dealloc_stack %tmp : $*NonTrivialStruct
1626+
dealloc_stack %src : $*NonTrivialStruct
1627+
%retval = tuple ()
1628+
return %retval : $()
1629+
}
1630+
15701631
// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_load_copy : {{.*}} {
15711632
// CHECK: [[GET:%[^,]+]] = function_ref @getKlass
15721633
// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result
@@ -1703,3 +1764,31 @@ entry(%instance : $*NonTrivialStruct):
17031764
%retval = tuple ()
17041765
return %retval : $()
17051766
}
1767+
1768+
// Verify that no copy of an instance of the move-only type MOS is introduced.
1769+
// CHECK-LABEL: sil hidden [ossa] @dont_copy_move_only_struct : {{.*}} {
1770+
// CHECK: [[SRC:%[^,]+]] = alloc_stack $MOS
1771+
// CHECK: [[GET:%[^,]+]] = function_ref @getMOS
1772+
// CHECK: [[INSTANCE_1:%[^,]+]] = apply [[GET]]()
1773+
// CHECK: store [[INSTANCE_1]] to [init] [[SRC]]
1774+
// CHECK: [[INSTANCE_2:%[^,]+]] = load [take] [[SRC]]
1775+
// CHECK: store [[INSTANCE_2]] to [init] [[SRC]]
1776+
// CHECK: [[INSTANCE_3:%[^,]+]] = load [take] [[SRC]]
1777+
// CHECK: dealloc_stack [[SRC]]
1778+
// CHECK: return [[INSTANCE_3]]
1779+
// CHECK-LABEL: } // end sil function 'dont_copy_move_only_struct'
1780+
sil hidden [ossa] @dont_copy_move_only_struct : $@convention(thin) () -> @owned MOS {
1781+
bb0:
1782+
%src = alloc_stack $MOS
1783+
%getMOS = function_ref @getMOS : $@convention(thin) () -> @owned MOS
1784+
%instance_1 = apply %getMOS() : $@convention(thin) () -> @owned MOS
1785+
store %instance_1 to [init] %src : $*MOS
1786+
%tmp = alloc_stack $MOS
1787+
copy_addr [take] %src to [init] %tmp : $*MOS
1788+
%instance_2 = load [take] %tmp : $*MOS
1789+
store %instance_2 to [init] %src : $*MOS
1790+
%instance_3 = load [take] %src : $*MOS
1791+
dealloc_stack %tmp : $*MOS
1792+
dealloc_stack %src : $*MOS
1793+
return %instance_3 : $MOS
1794+
}

0 commit comments

Comments
 (0)