From 5104c317caac8d50905e30371ee3f233a0d37716 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Fri, 10 Oct 2025 21:08:53 +0200 Subject: [PATCH] TempRValueElimination: fix a problem with non-copyable types In case of a non-copyable type the final destroy (or take) of a stack location can be missing if the value has only trivial fields. The optimization inserted a `destroy_addr` in this case although it wasn't there before. Beside fixing this problem I also refactored the code a bit to make it more readable. --- .../TempRValueElimination.swift | 108 +++++++++--------- test/SILOptimizer/temp_rvalue_opt_ossa.sil | 23 ++++ 2 files changed, 78 insertions(+), 53 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift index b5cac210cb359..dc908b7b6584e 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempRValueElimination.swift @@ -77,45 +77,19 @@ private func tryEliminate(copy: CopyLikeInstruction, keepDebugInfo: Bool, _ cont return } - if needToInsertDestroy(copy: copy, lastUseOfAllocStack: lastUseOfAllocStack) { - Builder.insert(after: lastUseOfAllocStack, context) { builder in - builder.createDestroyAddr(address: copy.sourceAddress) - } + if copy.isTakeOfSource { + hoistDestroy(of: allocStack, after: lastUseOfAllocStack, context) + } else { + removeDestroys(of: allocStack, context) } - for use in allocStack.uses { - switch use.instruction { - case is DestroyAddrInst, is DeallocStackInst: - context.erase(instruction: use.instruction) - case let cai as CopyAddrInst where cai != copy: - if cai.isTakeOfSource, !copy.isTakeOfSource { - // If the original copy is not taking its source, a `copy_addr` from the `allocStack` must - // not take its source either. - assert(cai == lastUseOfAllocStack && cai.source == allocStack) - cai.set(isTakeOfSource: false, context) - } - use.set(to: copy.sourceAddress, context) - case let load as LoadInst: - if load.loadOwnership == .take, !copy.isTakeOfSource { - // If the original copy is not taking its source, a `load` from the `allocStack` must - // not take its source either. - assert(load == lastUseOfAllocStack) - load.set(ownership: .copy, context) - } - use.set(to: copy.sourceAddress, context); + allocStack.uses.ignoreUses(ofType: DeallocStackInst.self).replaceAll(with: copy.sourceAddress, context) - default: - // Note that no operations that may be handled by this default clause can destroy the `allocStack`. - // This includes operations that load the value from memory and release it or cast the address - // before destroying it. - use.set(to: copy.sourceAddress, context); - } - } - context.erase(instruction: allocStack) if keepDebugInfo { Builder(before: copy, context).createDebugStep() } context.erase(instructionIncludingAllUsers: copy.loadingInstruction) + context.erase(instructionIncludingAllUsers: allocStack) } /// Checks if the `copy` is copying into an `alloc_stack` which is removable: @@ -193,32 +167,60 @@ private func getRemovableAllocStackDestination( return (allocStack, lastUseOfAllocStack) } -/// We need to insert a final destroy if the original `copy` is taking the source but the -/// `lastUseOfAllocStack` is not taking the `alloc_stack`. -private func needToInsertDestroy(copy: CopyLikeInstruction, lastUseOfAllocStack: Instruction) -> Bool { - if !copy.isTakeOfSource { - return false +// We need to hoist the destroy of the stack location up to its last use because the source of the copy +// could be re-initialized between the last use and the destroy. +private func hoistDestroy(of allocStack: AllocStackInst, after lastUse: Instruction, _ context: FunctionPassContext) { + // Check if the last use already takes the stack location, + switch lastUse { + case let cai as CopyAddrInst where cai.source == allocStack && cai.isTakeOfSource: + return + case let li as LoadInst where li.loadOwnership == .take: + assert(li.address == allocStack, "load must be not take a projected address") + return + default: + break } - switch lastUseOfAllocStack { - case copy: - return true - case let cai as CopyAddrInst: - if cai.isTakeOfSource { - assert(cai.source == copy.destinationAddress, "copy_addr must be not take a projected address") - return false - } - return true - case let li as LoadInst: - if li.loadOwnership == .take { - assert(li.address == copy.destinationAddress, "load must be not take a projected address") - return false + if allocStack.uses.users(ofType: DestroyAddrInst.self).isEmpty { + // The stack location is not destroyed at all! This can happen if it contains a non-copyable + // value which has only trivial fields. + return + } + + // Note that there can be multiple destroy_addr because they don't need to be in the same block + // as the alloc_stack, e.g. + // %1 = alloc_stack + // cond_br %c, bb1, bb2 + // bb1: + // destroy_addr %1 + // bb2: + // destroy_addr %1 + context.erase(instructions: allocStack.uses.users(ofType: DestroyAddrInst.self)) + + Builder.insert(after: lastUse, context) { builder in + builder.createDestroyAddr(address: allocStack) + } +} + +private func removeDestroys(of allocStack: AllocStackInst, _ context: FunctionPassContext) { + for use in allocStack.uses { + switch use.instruction { + case is DestroyAddrInst: + context.erase(instruction: use.instruction) + case let cai as CopyAddrInst where cai.isTakeOfSource: + assert(cai.source == allocStack, "partial takes of the stack location are not supported") + cai.set(isTakeOfSource: false, context) + case let load as LoadInst where load.loadOwnership == .take: + assert(load.address == allocStack, "partial takes of the stack location are not supported") + load.set(ownership: .copy, context) + default: + // Note that no operations other than the cases above can destroy the `allocStack` (we checked + // this in the `UseCollector`). + break } - return true - default: - return true } } + private extension AllocStackInst { func hasUses(before instruction: Instruction, _ context: FunctionPassContext) -> Bool { var useSet = InstructionSet(context) diff --git a/test/SILOptimizer/temp_rvalue_opt_ossa.sil b/test/SILOptimizer/temp_rvalue_opt_ossa.sil index 3dc34339b59d2..a6a42fc2903ff 100644 --- a/test/SILOptimizer/temp_rvalue_opt_ossa.sil +++ b/test/SILOptimizer/temp_rvalue_opt_ossa.sil @@ -42,6 +42,13 @@ public enum FakeOptional { struct MOS: ~Copyable {} +struct NCWithDeinit : ~Copyable { + var x: Int + + deinit +} + + protocol P { func foo() } @@ -1332,6 +1339,22 @@ sil [ossa] @take_from_original_copy_addr__final_use_apply__move_only : $() -> () return %tuple : $() } +// CHECK-LABEL: sil [ossa] @missing_destroy : +// CHECK-NOT: destroy_addr +// CHECK-NOT: copy_addr +// CHECK: = drop_deinit %0 +// CHECK-NEXT: = tuple () +// CHECK-LABEL: } // end sil function 'missing_destroy' +sil [ossa] @missing_destroy : $@convention(thin) (@in NCWithDeinit) -> () { +bb0(%0 : $*NCWithDeinit): + %1 = alloc_stack $NCWithDeinit + copy_addr [take] %0 to [init] %1 + %3 = drop_deinit %1 + dealloc_stack %1 + %5 = tuple () + return %5 +} + // CHECK-LABEL: sil [ossa] @test_temprvoborrowboundary1 : // CHECK-NOT: alloc_stack // CHECK-NOT: copy_addr