diff --git a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp index 3f2c291e09999..7cfa51bfc3c81 100644 --- a/lib/SILOptimizer/Transforms/TempRValueElimination.cpp +++ b/lib/SILOptimizer/Transforms/TempRValueElimination.cpp @@ -535,10 +535,11 @@ 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. - bool needToInsertDestroy = copyInst->isTakeOfSrc(); + // 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 // to the value initialized by this copy. It is sufficient to check that the @@ -557,7 +558,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 +589,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,6 +602,34 @@ void TempRValueOptPass::tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst) { LLVM_DEBUG(llvm::dbgs() << " Success: replace temp" << *tempObj); + // 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)) { + 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)) { + auto retval = li->getOperand() != tempObj || + li->getOwnershipQualifier() != LoadOwnershipQualifier::Take; + assert(!tempObj->getType().isMoveOnly() || + !retval && "introducing copy of move-only value!?"); + return retval; + } + return true; + }(); if (needToInsertDestroy) { // Compensate the [take] of the original copyInst. SILBuilderWithScope::insertAfter(lastLoadInst, [&] (SILBuilder &builder) { @@ -630,16 +659,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 f0009c8964909..a2c025b81da49 100644 --- a/test/SILOptimizer/temp_rvalue_opt_ossa.sil +++ b/test/SILOptimizer/temp_rvalue_opt_ossa.sil @@ -39,10 +39,18 @@ 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_MOS : $@convention(thin) (@in_guaranteed MOS) -> () sil [ossa] @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () { bb0(%0 : $*Klass): @@ -764,8 +772,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' @@ -1506,6 +1513,208 @@ 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 [take] [[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 [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 { +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_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 +// 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 : $() +} + +// 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 @@ -1584,3 +1793,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 +}