From 61335d789409d01cfbffaaae388d27341c275c90 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 10 Apr 2023 13:37:35 -0700 Subject: [PATCH 1/4] [TempRValueOpt] NFC: Renamed variable. In preparation for "folding" an "inserted destroy" into a load [copy] or copy_addr, rename the variable that indicates whether the copyInst's source must be deinitialized after its last "load". --- lib/SILOptimizer/Transforms/TempRValueElimination.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp index 3f2c291e09999..4f9551017653b 100644 --- a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp +++ b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp @@ -538,7 +538,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { // If the source of the copyInst is taken, we must insert a compensating // destroy_addr. This must be done at the right spot: after the last use // tempObj, but before any (potential) re-initialization of the source. - bool needToInsertDestroy = copyInst->isTakeOfSrc(); + bool needFinalDeinit = copyInst->isTakeOfSrc(); // Scan all uses of the temporary storage (tempObj) to verify they all refer // to the value initialized by this copy. It is sufficient to check that the @@ -557,7 +557,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { // Also, destroys are allowed to be in a different block. if (isa(user)) { - if (!isOSSA && needToInsertDestroy) { + if (!isOSSA && needFinalDeinit) { // In non-OSSA mode, for the purpose of inserting the destroy of // copySrc, we have to be conservative and assume that the lifetime of // tempObj goes beyond it's last use - until the final destroy_addr. @@ -588,7 +588,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { // Example: // copy_addr [take] %copySrc to [init] %tempObj // copyInst // copy_addr [take] %tempObj to [init] %copySrc // lastLoadInst - if (needToInsertDestroy && lastLoadInst != copyInst && + if (needFinalDeinit && lastLoadInst != copyInst && !isa(lastLoadInst) && aa->mayWriteToMemory(lastLoadInst, copySrc)) return; @@ -601,7 +601,7 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj); - if (needToInsertDestroy) { + if (needFinalDeinit) { // Compensate the [take] of the original copyInst. SILBuilderWithScope::insertAfter(lastLoadInst, [&] (SILBuilder &builder) { builder.createDestroyAddr(builder.getInsertionPoint()->getLoc(), copySrc); From e3163bbccdab48c1942cdff487566e7d08c50e84 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 10 Apr 2023 14:52:18 -0700 Subject: [PATCH 2/4] [Test] Added tests showing extra copies. As a separate commit to make the behavior change more obvious. --- test/SILOptimizer/temp_rvalue_opt_ossa.sil | 119 +++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/test/SILOptimizer/temp_rvalue_opt_ossa.sil b/test/SILOptimizer/temp_rvalue_opt_ossa.sil index f0009c8964909..fb8f121215e3a 100644 --- a/test/SILOptimizer/temp_rvalue_opt_ossa.sil +++ b/test/SILOptimizer/temp_rvalue_opt_ossa.sil @@ -39,6 +39,9 @@ public enum FakeOptional { case some(T) } + +sil [ossa] @getKlass : $@convention(thin) () -> @owned Klass + sil @unknown : $@convention(thin) () -> () sil [ossa] @guaranteed_user : $@convention(thin) (@guaranteed Klass) -> () @@ -1506,6 +1509,122 @@ bb2: unwind } +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_load_take : {{.*}} { +// CHECK: [[GET:%[^,]+]] = function_ref @getKlass +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result +// CHECK: [[INSTANCE_1:%[^,]+]] = apply [[GET]]() +// CHECK: [[ADDR:%[^,]+]] = alloc_stack $Klass +// CHECK: store [[INSTANCE_1]] to [init] [[ADDR]] +// CHECK: apply [[USER]]([[ADDR]]) +// CHECK: [[INSTANCE_2:%[^,]+]] = load [copy] [[ADDR]] +// Extra copy: ^^^^^^^^^^^ +// CHECK: destroy_addr [[ADDR]] +// CHECK: dealloc_stack [[ADDR]] +// CHECK: return [[INSTANCE_2]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_load_take' +sil [ossa] @take_from_original_copy_addr__final_use_load_take : $() -> @owned Klass { + %getKlass = function_ref @getKlass : $@convention(thin) () -> @owned Klass + %user = function_ref @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () + %instance_1 = apply %getKlass() : $@convention(thin) () -> @owned Klass + %src = alloc_stack $Klass + store %instance_1 to [init] %src : $*Klass + apply %user(%src) : $@convention(thin) (@in_guaranteed Klass) -> () + %tmp = alloc_stack $Klass + copy_addr [take] %src to [init] %tmp : $*Klass + %instance_2 = load [take] %tmp : $*Klass + dealloc_stack %tmp : $*Klass + dealloc_stack %src : $*Klass + return %instance_2 : $Klass +} + +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_take : {{.*}} { +// CHECK: {{bb[0-9]+}}([[OUT:%[^,]+]] : +// CHECK: [[GET:%[^,]+]] = function_ref @getKlass +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result +// CHECK: [[INSTANCE_1:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $Klass +// CHECK: store [[INSTANCE_1]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: copy_addr [[SRC]] to [init] [[OUT]] +// Extra copy: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_copy_addr_take' +sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_take : $() -> @out Klass { +entry(%out : $*Klass): + %getKlass = function_ref @getKlass : $@convention(thin) () -> @owned Klass + %user = function_ref @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () + %instance_1 = apply %getKlass() : $@convention(thin) () -> @owned Klass + %src = alloc_stack $Klass + store %instance_1 to [init] %src : $*Klass + apply %user(%src) : $@convention(thin) (@in_guaranteed Klass) -> () + %tmp = alloc_stack $Klass + copy_addr [take] %src to [init] %tmp : $*Klass + copy_addr [take] %tmp to [init] %out : $*Klass + dealloc_stack %tmp : $*Klass + dealloc_stack %src : $*Klass + %retval = tuple () + return %retval : $() +} + +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_load_copy : {{.*}} { +// CHECK: [[GET:%[^,]+]] = function_ref @getKlass +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result +// CHECK: [[INSTANCE_1:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $Klass +// CHECK: store [[INSTANCE_1]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: [[INSTANCE_2:%[^,]+]] = load [copy] [[SRC]] +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK: return [[INSTANCE_2]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_load_copy' +sil [ossa] @take_from_original_copy_addr__final_use_load_copy : $() -> @owned Klass { + %getKlass = function_ref @getKlass : $@convention(thin) () -> @owned Klass + %user = function_ref @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () + %instance_1 = apply %getKlass() : $@convention(thin) () -> @owned Klass + %src = alloc_stack $Klass + store %instance_1 to [init] %src : $*Klass + apply %user(%src) : $@convention(thin) (@in_guaranteed Klass) -> () + %tmp = alloc_stack $Klass + copy_addr [take] %src to [init] %tmp : $*Klass + %instance_2 = load [copy] %tmp : $*Klass + destroy_addr %tmp : $*Klass + dealloc_stack %tmp : $*Klass + dealloc_stack %src : $*Klass + return %instance_2 : $Klass +} + +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_copy : {{.*}} { +// CHECK: {{bb[0-9]+}}([[OUT:%[^,]+]] : $*Klass): +// CHECK: [[GET:%[^,]+]] = function_ref @getKlass +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result +// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $Klass +// CHECK: store [[INSTANCE]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: copy_addr [[SRC]] to [init] [[OUT]] +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_copy_addr_copy' +sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_copy : $() -> @out Klass { +entry(%out : $*Klass): + %getKlass = function_ref @getKlass : $@convention(thin) () -> @owned Klass + %user = function_ref @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () + %instance_1 = apply %getKlass() : $@convention(thin) () -> @owned Klass + %src = alloc_stack $Klass + store %instance_1 to [init] %src : $*Klass + apply %user(%src) : $@convention(thin) (@in_guaranteed Klass) -> () + %tmp = alloc_stack $Klass + copy_addr [take] %src to [init] %tmp : $*Klass + copy_addr %tmp to [init] %out : $*Klass + destroy_addr %tmp : $*Klass + dealloc_stack %tmp : $*Klass + dealloc_stack %src : $*Klass + %retval = tuple () + return %retval : $() +} + // This does not get optimized correctly because of the conservative treatment of load_borrow/end_borrow in MemBehavior // CHECK-LABEL: sil [ossa] @test_temprvoborrowboundary1 : // CHECK: copy_addr From a5f4a16b525774ccf07d3b81dcb6c14625cb5e19 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 10 Apr 2023 13:40:54 -0700 Subject: [PATCH 3/4] [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 --- .../Transforms/TempRValueElimination.cpp | 38 ++++++- test/SILOptimizer/temp_rvalue_opt_ossa.sil | 105 ++++++++++++++++-- 2 files changed, 129 insertions(+), 14 deletions(-) diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp index 4f9551017653b..0d76b035b58de 100644 --- a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp +++ b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp @@ -535,9 +535,10 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { SILValue copySrc = copyInst->getSrc(); assert(tempObj != copySrc && "can't initialize temporary with itself"); - // If the source of the copyInst is taken, we must insert a compensating - // destroy_addr. This must be done at the right spot: after the last use - // tempObj, but before any (potential) re-initialization of the source. + // If the source of the copyInst is taken, it must be deinitialized (via + // destroy_addr, load [take], copy_addr [take]). This must be done at the + // right spot: after the last use tempObj, but before any (potential) + // re-initialization of the source. bool needFinalDeinit = copyInst->isTakeOfSrc(); // Scan all uses of the temporary storage (tempObj) to verify they all refer @@ -601,7 +602,29 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj); - if (needFinalDeinit) { + // If copyInst's source must be deinitialized, whether that must be done via + // a newly created destroy_addr. + // + // If lastLoadInst is a load or a copy_addr, then the deinitialization can be + // done in that instruction. + // + // This is necessary for correctness: otherwise, copies of move-only values + // would be introduced. + bool needToInsertDestroy = [&]() { + if (!needFinalDeinit) + return false; + if (lastLoadInst == copyInst) + return true; + if (auto *cai = dyn_cast(lastLoadInst)) { + return cai->getSrc() != tempObj || !cai->isTakeOfSrc(); + } + if (auto *li = dyn_cast(lastLoadInst)) { + return li->getOperand() != tempObj || + li->getOwnershipQualifier() != LoadOwnershipQualifier::Take; + } + return true; + }(); + if (needToInsertDestroy) { // Compensate the [take] of the original copyInst. SILBuilderWithScope::insertAfter(lastLoadInst, [&] (SILBuilder &builder) { builder.createDestroyAddr(builder.getInsertionPoint()->getLoc(), copySrc); @@ -630,16 +653,19 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { auto *cai = cast(user); if (cai != copyInst) { assert(cai->getSrc() == tempObj); - if (cai->isTakeOfSrc()) + if (cai->isTakeOfSrc() && (!needFinalDeinit || lastLoadInst != cai)) { cai->setIsTakeOfSrc(IsNotTake); + } } use->set(copySrc); break; } case SILInstructionKind::LoadInst: { auto *li = cast(user); - if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) + if (li->getOwnershipQualifier() == LoadOwnershipQualifier::Take && + (!needFinalDeinit || li != lastLoadInst)) { li->setOwnershipQualifier(LoadOwnershipQualifier::Copy); + } use->set(copySrc); break; } diff --git a/test/SILOptimizer/temp_rvalue_opt_ossa.sil b/test/SILOptimizer/temp_rvalue_opt_ossa.sil index fb8f121215e3a..9fe4c3e2023c8 100644 --- a/test/SILOptimizer/temp_rvalue_opt_ossa.sil +++ b/test/SILOptimizer/temp_rvalue_opt_ossa.sil @@ -39,13 +39,17 @@ public enum FakeOptional { case some(T) } +@_moveOnly struct MOS {} sil [ossa] @getKlass : $@convention(thin) () -> @owned Klass +sil [ossa] @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct +sil [ossa] @getMOS : $@convention(thin) () -> @owned MOS sil @unknown : $@convention(thin) () -> () sil [ossa] @guaranteed_user : $@convention(thin) (@guaranteed Klass) -> () sil [ossa] @guaranteed_user_with_result : $@convention(thin) (@guaranteed Klass) -> @out Klass +sil [ossa] @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () { bb0(%0 : $*Klass): @@ -767,8 +771,7 @@ bb0(%0 : $*Builtin.NativeObject): // // CHECK-LABEL: sil [ossa] @takeWithLoadRelease : $@convention(thin) (@in Builtin.NativeObject) -> () { // CHECK: bb0(%0 : $*Builtin.NativeObject): -// CHECK: [[V:%.*]] = load [copy] %0 : $*Builtin.NativeObject -// CHECK: destroy_addr %0 : $*Builtin.NativeObject +// CHECK: [[V:%.*]] = load [take] %0 : $*Builtin.NativeObject // CHECK: apply %{{.*}}([[V]]) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () // CHECK: destroy_value [[V]] : $Builtin.NativeObject // CHECK-LABEL: } // end sil function 'takeWithLoadRelease' @@ -1516,9 +1519,7 @@ bb2: // CHECK: [[ADDR:%[^,]+]] = alloc_stack $Klass // CHECK: store [[INSTANCE_1]] to [init] [[ADDR]] // CHECK: apply [[USER]]([[ADDR]]) -// CHECK: [[INSTANCE_2:%[^,]+]] = load [copy] [[ADDR]] -// Extra copy: ^^^^^^^^^^^ -// CHECK: destroy_addr [[ADDR]] +// CHECK: [[INSTANCE_2:%[^,]+]] = load [take] [[ADDR]] // CHECK: dealloc_stack [[ADDR]] // CHECK: return [[INSTANCE_2]] // 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 // CHECK: [[SRC:%[^,]+]] = alloc_stack $Klass // CHECK: store [[INSTANCE_1]] to [init] [[SRC]] // CHECK: apply [[USER]]([[SRC]]) -// CHECK: copy_addr [[SRC]] to [init] [[OUT]] -// Extra copy: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -// CHECK: destroy_addr [[SRC]] +// CHECK: copy_addr [take] [[SRC]] to [init] [[OUT]] // CHECK: dealloc_stack [[SRC]] // CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_copy_addr_take' sil [ossa] @take_from_original_copy_addr__final_use_copy_addr_take : $() -> @out Klass { @@ -1567,6 +1566,68 @@ entry(%out : $*Klass): return %retval : $() } +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_field_load_copy : {{.*}} { +// CHECK: [[GET:%[^,]+]] = function_ref @getNonTrivialStruct +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result_NTS +// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $NonTrivialStruct +// CHECK: store [[INSTANCE]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: [[FIELD_ADDR:%[^,]+]] = struct_element_addr [[SRC]] +// CHECK: [[FIELD:%[^,]+]] = load [copy] [[FIELD_ADDR]] +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK: return [[FIELD]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_field_load_copy' +sil [ossa] @take_from_original_copy_addr__final_use_field_load_copy : $() -> @owned Klass { + %get = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct + %user = function_ref @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () + %instance_1 = apply %get() : $@convention(thin) () -> @owned NonTrivialStruct + %src = alloc_stack $NonTrivialStruct + store %instance_1 to [init] %src : $*NonTrivialStruct + apply %user(%src) : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () + %tmp = alloc_stack $NonTrivialStruct + copy_addr [take] %src to [init] %tmp : $*NonTrivialStruct + %field_addr = struct_element_addr %tmp : $*NonTrivialStruct, #NonTrivialStruct.val + %field = load [copy] %field_addr : $*Klass + destroy_addr %tmp : $*NonTrivialStruct + dealloc_stack %tmp : $*NonTrivialStruct + dealloc_stack %src : $*NonTrivialStruct + return %field : $Klass +} + +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_field_copy_addr_take : {{.*}} { +// CHECK: {{bb[0-9]+}}([[OUT:%[^,]+]] : +// CHECK: [[GET:%[^,]+]] = function_ref @getNonTrivialStruct +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result_NTS +// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $NonTrivialStruct +// CHECK: store [[INSTANCE]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: [[FIELD_ADDR:%[^,]+]] = struct_element_addr [[SRC]] +// CHECK: copy_addr [[FIELD_ADDR]] to [init] [[OUT]] +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_field_copy_addr_take' +sil [ossa] @take_from_original_copy_addr__final_use_field_copy_addr_take : $() -> @out Klass { +entry(%out : $*Klass): + %getNonTrivialStruct = function_ref @getNonTrivialStruct : $@convention(thin) () -> @owned NonTrivialStruct + %user = function_ref @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () + %instance_1 = apply %getNonTrivialStruct() : $@convention(thin) () -> @owned NonTrivialStruct + %src = alloc_stack $NonTrivialStruct + store %instance_1 to [init] %src : $*NonTrivialStruct + apply %user(%src) : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () + %tmp = alloc_stack $NonTrivialStruct + copy_addr [take] %src to [init] %tmp : $*NonTrivialStruct + %field_addr = struct_element_addr %tmp : $*NonTrivialStruct, #NonTrivialStruct.val + copy_addr %field_addr to [init] %out : $*Klass + destroy_addr %tmp : $*NonTrivialStruct + dealloc_stack %tmp : $*NonTrivialStruct + dealloc_stack %src : $*NonTrivialStruct + %retval = tuple () + return %retval : $() +} + // CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_load_copy : {{.*}} { // CHECK: [[GET:%[^,]+]] = function_ref @getKlass // CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result @@ -1703,3 +1764,31 @@ entry(%instance : $*NonTrivialStruct): %retval = tuple () return %retval : $() } + +// Verify that no copy of an instance of the move-only type MOS is introduced. +// CHECK-LABEL: sil hidden [ossa] @dont_copy_move_only_struct : {{.*}} { +// CHECK: [[SRC:%[^,]+]] = alloc_stack $MOS +// CHECK: [[GET:%[^,]+]] = function_ref @getMOS +// CHECK: [[INSTANCE_1:%[^,]+]] = apply [[GET]]() +// CHECK: store [[INSTANCE_1]] to [init] [[SRC]] +// CHECK: [[INSTANCE_2:%[^,]+]] = load [take] [[SRC]] +// CHECK: store [[INSTANCE_2]] to [init] [[SRC]] +// CHECK: [[INSTANCE_3:%[^,]+]] = load [take] [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK: return [[INSTANCE_3]] +// CHECK-LABEL: } // end sil function 'dont_copy_move_only_struct' +sil hidden [ossa] @dont_copy_move_only_struct : $@convention(thin) () -> @owned MOS { +bb0: + %src = alloc_stack $MOS + %getMOS = function_ref @getMOS : $@convention(thin) () -> @owned MOS + %instance_1 = apply %getMOS() : $@convention(thin) () -> @owned MOS + store %instance_1 to [init] %src : $*MOS + %tmp = alloc_stack $MOS + copy_addr [take] %src to [init] %tmp : $*MOS + %instance_2 = load [take] %tmp : $*MOS + store %instance_2 to [init] %src : $*MOS + %instance_3 = load [take] %src : $*MOS + dealloc_stack %tmp : $*MOS + dealloc_stack %src : $*MOS + return %instance_3 : $MOS +} From 315c3c9024830526219295d16f6b97d3ed68e4fb Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Tue, 11 Apr 2023 10:34:54 -0700 Subject: [PATCH 4/4] [TempRValueOpt] NFC: Assert no copy of uncopyable. --- .../Transforms/TempRValueElimination.cpp | 12 ++++++-- test/SILOptimizer/temp_rvalue_opt_ossa.sil | 29 +++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp index 0d76b035b58de..7cfa51bfc3c81 100644 --- a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp +++ b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp @@ -616,11 +616,17 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { if (lastLoadInst == copyInst) return true; if (auto *cai = dyn_cast(lastLoadInst)) { - return cai->getSrc() != tempObj || !cai->isTakeOfSrc(); + auto retval = cai->getSrc() != tempObj || !cai->isTakeOfSrc(); + assert(!tempObj->getType().isMoveOnly() || + !retval && "introducing copy of move-only value!?"); + return retval; } if (auto *li = dyn_cast(lastLoadInst)) { - return li->getOperand() != tempObj || - li->getOwnershipQualifier() != LoadOwnershipQualifier::Take; + auto retval = li->getOperand() != tempObj || + li->getOwnershipQualifier() != LoadOwnershipQualifier::Take; + assert(!tempObj->getType().isMoveOnly() || + !retval && "introducing copy of move-only value!?"); + return retval; } return true; }(); diff --git a/test/SILOptimizer/temp_rvalue_opt_ossa.sil b/test/SILOptimizer/temp_rvalue_opt_ossa.sil index 9fe4c3e2023c8..a2c025b81da49 100644 --- a/test/SILOptimizer/temp_rvalue_opt_ossa.sil +++ b/test/SILOptimizer/temp_rvalue_opt_ossa.sil @@ -50,6 +50,7 @@ sil @unknown : $@convention(thin) () -> () sil [ossa] @guaranteed_user : $@convention(thin) (@guaranteed Klass) -> () sil [ossa] @guaranteed_user_with_result : $@convention(thin) (@guaranteed Klass) -> @out Klass sil [ossa] @inguaranteed_user_without_result_NTS : $@convention(thin) (@in_guaranteed NonTrivialStruct) -> () +sil [ossa] @inguaranteed_user_without_result_MOS : $@convention(thin) (@in_guaranteed MOS) -> () sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () { bb0(%0 : $*Klass): @@ -1686,6 +1687,34 @@ entry(%out : $*Klass): return %retval : $() } +// CHECK-LABEL: sil [ossa] @take_from_original_copy_addr__final_use_apply__move_only : {{.*}} { +// CHECK: [[GET:%[^,]+]] = function_ref @getMOS +// CHECK: [[USER:%[^,]+]] = function_ref @inguaranteed_user_without_result_MOS +// CHECK: [[INSTANCE:%[^,]+]] = apply [[GET]]() +// CHECK: [[SRC:%[^,]+]] = alloc_stack $MOS +// CHECK: store [[INSTANCE]] to [init] [[SRC]] +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: apply [[USER]]([[SRC]]) +// CHECK: destroy_addr [[SRC]] +// CHECK: dealloc_stack [[SRC]] +// CHECK-LABEL: } // end sil function 'take_from_original_copy_addr__final_use_apply__move_only' +sil [ossa] @take_from_original_copy_addr__final_use_apply__move_only : $() -> () { + %getMOS = function_ref @getMOS : $@convention(thin) () -> @owned MOS + %user = function_ref @inguaranteed_user_without_result_MOS : $@convention(thin) (@in_guaranteed MOS) -> () + %instance_1 = apply %getMOS() : $@convention(thin) () -> @owned MOS + %src = alloc_stack $MOS + store %instance_1 to [init] %src : $*MOS + apply %user(%src) : $@convention(thin) (@in_guaranteed MOS) -> () + %tmp = alloc_stack $MOS + copy_addr [take] %src to [init] %tmp : $*MOS + apply %user(%tmp) : $@convention(thin) (@in_guaranteed MOS) -> () + destroy_addr %tmp : $*MOS + dealloc_stack %tmp : $*MOS + dealloc_stack %src : $*MOS + %tuple = tuple () + return %tuple : $() +} + // This does not get optimized correctly because of the conservative treatment of load_borrow/end_borrow in MemBehavior // CHECK-LABEL: sil [ossa] @test_temprvoborrowboundary1 : // CHECK: copy_addr