From 7bbe5c6fe2ab8d15f0e973ff1f480b56a2c3945b Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Tue, 30 Sep 2025 08:47:16 +0200 Subject: [PATCH 1/4] LoopInvariantCodeMotion: don't hoist loads and stores if the memory location is not initialized at loop exits. If the memory is not initialized at all exits, it would be wrong to insert stores at exit blocks. --- .../LoopInvariantCodeMotion.swift | 40 ++++++++++ .../Sources/SIL/DataStructures/Worklist.swift | 4 +- test/SILOptimizer/licm.sil | 78 +++++++++++++++++++ 3 files changed, 120 insertions(+), 2 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift index 8012c8d93cd58..5b86a2d65cb82 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift @@ -693,6 +693,12 @@ private extension MovableInstructions { accessPath: AccessPath, context: FunctionPassContext ) -> Bool { + + // If the memory is not initialized at all exits, it would be wrong to insert stores at exit blocks. + guard memoryIsInitializedAtAllExits(of: loop, accessPath: accessPath, context) else { + return false + } + // Initially load the value in the loop pre header. let builder = Builder(before: loop.preheader!.terminator, context) var firstStore: StoreInst? @@ -831,6 +837,40 @@ private extension MovableInstructions { return changed } + + func memoryIsInitializedAtAllExits(of loop: Loop, accessPath: AccessPath, _ context: FunctionPassContext) -> Bool { + + // Perform a simple dataflow analysis which checks if there is a path from a `load [take]` + // (= the only kind of instruction which can de-initialize the memory) to a loop exit without + // a `store` in between. + + var stores = InstructionSet(context) + defer { stores.deinitialize() } + for case let store as StoreInst in loadsAndStores where store.storesTo(accessPath) { + stores.insert(store) + } + + var exitInsts = InstructionSet(context) + defer { exitInsts.deinitialize() } + exitInsts.insert(contentsOf: loop.exitBlocks.lazy.map { $0.instructions.first! }) + + var worklist = InstructionWorklist(context) + defer { worklist.deinitialize() } + for case let load as LoadInst in loadsAndStores where load.loadOwnership == .take && load.loadsFrom(accessPath) { + worklist.pushIfNotVisited(load) + } + + while let inst = worklist.pop() { + if stores.contains(inst) { + continue + } + if exitInsts.contains(inst) { + return false + } + worklist.pushSuccessors(of: inst) + } + return true + } } private extension Instruction { diff --git a/SwiftCompilerSources/Sources/SIL/DataStructures/Worklist.swift b/SwiftCompilerSources/Sources/SIL/DataStructures/Worklist.swift index 353b38dc19509..acdbe882c7a4a 100644 --- a/SwiftCompilerSources/Sources/SIL/DataStructures/Worklist.swift +++ b/SwiftCompilerSources/Sources/SIL/DataStructures/Worklist.swift @@ -77,7 +77,7 @@ public typealias ValueWorklist = Worklist public typealias OperandWorklist = Worklist extension InstructionWorklist { - public mutating func pushPredecessors(of inst: Instruction, ignoring ignoreInst: Instruction) { + public mutating func pushPredecessors(of inst: Instruction, ignoring ignoreInst: Instruction? = nil) { if let prev = inst.previous { if prev != ignoreInst { pushIfNotVisited(prev) @@ -92,7 +92,7 @@ extension InstructionWorklist { } } - public mutating func pushSuccessors(of inst: Instruction, ignoring ignoreInst: Instruction) { + public mutating func pushSuccessors(of inst: Instruction, ignoring ignoreInst: Instruction? = nil) { if let succ = inst.next { if succ != ignoreInst { pushIfNotVisited(succ) diff --git a/test/SILOptimizer/licm.sil b/test/SILOptimizer/licm.sil index 24be1f0614c6a..7ddde7ce61a10 100644 --- a/test/SILOptimizer/licm.sil +++ b/test/SILOptimizer/licm.sil @@ -1927,3 +1927,81 @@ bb3: %13 = tuple () return %13 } + +// Just check that LICM doesn't produce invalid SIL. +// CHECK-LABEL: sil [ossa] @uninitialized_at_entry_and_exit : +// CHECK-LABEL: } // end sil function 'uninitialized_at_entry_and_exit' +sil [ossa] @uninitialized_at_entry_and_exit : $@convention(thin) (@owned S) -> @out S { +bb0(%0 : $*S, %1 : @owned $S): + br bb1 + +bb1: + %5 = copy_value %1 + store %5 to [init] %0 + %7 = load [take] %0 + destroy_value %7 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + store %1 to [init] %0 + %r = tuple () + return %r +} + +// CHECK-LABEL: sil [ossa] @uninitialized_at_entry : +// CHECK: bb1: +// CHECK-NEXT: [[C:%.*]] = copy_value %1 +// CHECK-NEXT: cond_br +// CHECK: bb2: +// CHECK-NEXT: destroy_value [[C]] +// CHECK: bb3: +// CHECK-NEXT: store [[C]] to [init] %0 +// CHECK-LABEL: } // end sil function 'uninitialized_at_entry' +sil [ossa] @uninitialized_at_entry : $@convention(thin) (@owned S) -> @out S { +bb0(%0 : $*S, %1 : @owned $S): + br bb1 + +bb1: + %5 = copy_value %1 + store %5 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + %7 = load [take] %0 + destroy_value %7 + br bb1 + +bb3: + destroy_value %1 + %r = tuple () + return %r +} + +// Just check that LICM doesn't produce invalid SIL. +// CHECK-LABEL: sil [ossa] @uninitialized_at_exit : +// CHECK-LABEL: } // end sil function 'uninitialized_at_exit' +sil [ossa] @uninitialized_at_exit : $@convention(thin) (@in S) -> () { +bb0(%0 : $*S): + br bb1 + +bb1: + %4 = load [take] %0 + %5 = move_value %4 + store %5 to [init] %0 + %6 = load [take] %0 + cond_br undef, bb2, bb3 + +bb2: + %8 = move_value %6 + store %8 to [init] %0 + br bb1 + +bb3: + destroy_value %6 + %r = tuple () + return %r +} + From a6fb1876ef9fa794ba28c119b9af31624d9772c5 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Tue, 30 Sep 2025 09:38:30 +0200 Subject: [PATCH 2/4] LoopInvariantCodeMotion: correctly create projections for owned values Owned structs and tuples must be projected by `destructure_struct` and `destructure_tuple`, respectively. rdar://161467837 --- .../Optimizer/Utilities/OptUtils.swift | 20 +++++++++++--- test/SILOptimizer/licm.sil | 27 +++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift index dc49c0f238101..1dcedbd60049a 100644 --- a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift +++ b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift @@ -125,20 +125,32 @@ extension Value { return true } + /// Project out a sub-field of this value according to `path`. + /// If this is an "owned" value the result is an "owned" value which forwards the original value. + /// This only works if _all_ non-trivial fields are projected. Otherwise some non-trivial results of + /// `destructure_struct` or `destructure_tuple` will be leaked. func createProjection(path: SmallProjectionPath, builder: Builder) -> Value { let (kind, index, subPath) = path.pop() + let result: Value switch kind { case .root: return self case .structField: - let structExtract = builder.createStructExtract(struct: self, fieldIndex: index) - return structExtract.createProjection(path: subPath, builder: builder) + if ownership == .owned { + result = builder.createDestructureStruct(struct: self).results[index] + } else { + result = builder.createStructExtract(struct: self, fieldIndex: index) + } case .tupleField: - let tupleExtract = builder.createTupleExtract(tuple: self, elementIndex: index) - return tupleExtract.createProjection(path: subPath, builder: builder) + if ownership == .owned { + result = builder.createDestructureTuple(tuple: self).results[index] + } else { + result = builder.createTupleExtract(tuple: self, elementIndex: index) + } default: fatalError("path is not materializable") } + return result.createProjection(path: subPath, builder: builder) } func createAddressProjection(path: SmallProjectionPath, builder: Builder) -> Value { diff --git a/test/SILOptimizer/licm.sil b/test/SILOptimizer/licm.sil index 7ddde7ce61a10..780fbf27308f9 100644 --- a/test/SILOptimizer/licm.sil +++ b/test/SILOptimizer/licm.sil @@ -2005,3 +2005,30 @@ bb3: return %r } +// CHECK-LABEL: sil [ossa] @projected_load_take : +// CHECK: bb1([[V:%.*]] : @owned $(S, Int)): +// CHECK-NEXT: ([[E:%.*]], {{%[0-9]+}}) = destructure_tuple [[V]] +// CHECK-NEXT: ({{%[0-9]+}}, [[S:%.*]]) = destructure_struct [[E]] +// CHECK-NEXT: = struct $S (%1 : $Int, [[S]] : $String) +// CHECK-LABEL: } // end sil function 'projected_load_take' +sil [ossa] @projected_load_take : $@convention(thin) (@inout (S, Int), Int) -> () { +bb0(%0 : $*(S, Int), %1 : $Int): + br bb1 + +bb1: + %2 = tuple_element_addr %0, 0 + %3 = struct_element_addr %2, #S.s + %4 = load [take] %3 + %5 = struct $S (%1, %4) + %6 = tuple (%5, %1) + store %6 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + %r = tuple () + return %r +} + From fc6302e3e92ce050e222172720eef80a36bb89a2 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Tue, 30 Sep 2025 09:57:46 +0200 Subject: [PATCH 3/4] LoopInvariantCodeMotion: correctly handle `load [copy]` When moving loads and stores out of a loop, a `load [copy]` must be replaced by a `copy_value`. --- .../LoopInvariantCodeMotion.swift | 14 ++++- test/SILOptimizer/licm.sil | 56 +++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift index 5b86a2d65cb82..29e838aa4de31 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift @@ -798,7 +798,13 @@ private extension MovableInstructions { let rootVal = currentVal ?? ssaUpdater.getValue(inMiddleOf: block) if loadInst.operand.value.accessPath == accessPath { - loadInst.replace(with: rootVal, context) + if loadInst.loadOwnership == .copy { + let builder = Builder(before: loadInst, context) + let copy = builder.createCopyValue(operand: rootVal) + loadInst.replace(with: copy, context) + } else { + loadInst.replace(with: rootVal, context) + } changed = true continue } @@ -808,7 +814,11 @@ private extension MovableInstructions { } let builder = Builder(before: loadInst, context) - let projection = rootVal.createProjection(path: projectionPath, builder: builder) + let projection = if loadInst.loadOwnership == .copy { + rootVal.createProjectionAndCopy(path: projectionPath, builder: builder) + } else { + rootVal.createProjection(path: projectionPath, builder: builder) + } loadInst.replace(with: projection, context) changed = true diff --git a/test/SILOptimizer/licm.sil b/test/SILOptimizer/licm.sil index 780fbf27308f9..4d721734f5842 100644 --- a/test/SILOptimizer/licm.sil +++ b/test/SILOptimizer/licm.sil @@ -2032,3 +2032,59 @@ bb3: return %r } +// CHECK-LABEL: sil [ossa] @load_copy_followed_by_take : +// CHECK: bb1([[V:%.*]] : @owned $S): +// CHECK-NEXT: [[C:%.*]] = copy_value [[V]] +// CHECK-NEXT: destroy_value [[C]] +// CHECK-NEXT: = move_value [[V]] +// CHECK-LABEL: } // end sil function 'load_copy_followed_by_take' +sil [ossa] @load_copy_followed_by_take : $@convention(thin) (@inout S) -> () { +bb0(%0 : $*S): + br bb1 + +bb1: + %4 = load [copy] %0 + destroy_value %4 + %6 = load [take] %0 + %7 = move_value %6 + store %7 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + %r = tuple () + return %r +} + +// CHECK-LABEL: sil [ossa] @projected_load_copy_followed_by_take : +// CHECK: bb1([[V:%.*]] : @owned $S): +// CHECK-NEXT: [[B:%.*]] = begin_borrow [[V]] +// CHECK-NEXT: [[SE:%.*]] = struct_extract [[B]] +// CHECK-NEXT: [[C:%.*]] = copy_value [[SE]] +// CHECK-NEXT: end_borrow [[B]] +// CHECK-NEXT: destroy_value [[C]] +// CHECK-NEXT: ({{%[0-9]+}}, [[S:%.*]]) = destructure_struct [[V]] +// CHECK-NEXT: = struct $S (%1 : $Int, [[S]] : $String) +// CHECK-LABEL: } // end sil function 'projected_load_copy_followed_by_take' +sil [ossa] @projected_load_copy_followed_by_take : $@convention(thin) (@inout S, Int) -> () { +bb0(%0 : $*S, %1 : $Int): + br bb1 + +bb1: + %43 = struct_element_addr %0, #S.s + %4 = load [copy] %43 + destroy_value %4 + %48 = load [take] %43 + %49 = struct $S (%1, %48) + store %49 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + %r = tuple () + return %r +} From b527020364a4567f4f4cf81875dabe2004ce1cf8 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Tue, 30 Sep 2025 10:08:46 +0200 Subject: [PATCH 4/4] LoopInvariantCodeMotion: bail on split `load [take]` We currently don't support split `load [take]`, i.e. `load [take]` which does _not_ load all non-trivial fields of the initial value. --- .../LoopInvariantCodeMotion.swift | 42 ++++++++++++- test/SILOptimizer/licm.sil | 63 +++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift index 29e838aa4de31..6683098f8db07 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift @@ -727,7 +727,18 @@ private extension MovableInstructions { guard let firstStore else { return false } - + + // We currently don't support split `load [take]`, i.e. `load [take]` which does _not_ load all + // non-trivial fields of the initial value. + for case let load as LoadInst in loadsAndStores { + if load.loadOwnership == .take, + let path = accessPath.getProjection(to: load.address.accessPath), + !firstStore.destination.type.isProjectingEntireNonTrivialMembers(path: path, in: load.parentFunction) + { + return false + } + } + var ssaUpdater = SSAUpdater( function: firstStore.parentFunction, type: firstStore.destination.type.objectType, @@ -883,6 +894,35 @@ private extension MovableInstructions { } } +private extension Type { + func isProjectingEntireNonTrivialMembers(path: SmallProjectionPath, in function: Function) -> Bool { + let (kind, index, subPath) = path.pop() + switch kind { + case .root: + return true + case .structField: + guard let fields = getNominalFields(in: function) else { + return false + } + for (fieldIdx, fieldType) in fields.enumerated() { + if fieldIdx != index && !fieldType.isTrivial(in: function) { + return false + } + } + return fields[index].isProjectingEntireNonTrivialMembers(path: subPath, in: function) + case .tupleField: + for (elementIdx, elementType) in tupleElements.enumerated() { + if elementIdx != index && !elementType.isTrivial(in: function) { + return false + } + } + return tupleElements[index].isProjectingEntireNonTrivialMembers(path: subPath, in: function) + default: + fatalError("path is not materializable") + } + } +} + private extension Instruction { /// Returns `true` if this instruction follows the default hoisting heuristic which means it /// is not a terminator, allocation or deallocation and either a hoistable array semantics call or doesn't have memory effects. diff --git a/test/SILOptimizer/licm.sil b/test/SILOptimizer/licm.sil index 4d721734f5842..60b23fe3321a3 100644 --- a/test/SILOptimizer/licm.sil +++ b/test/SILOptimizer/licm.sil @@ -23,6 +23,12 @@ struct S { var s: String } +struct S2 { + var i: Int + var s1: String + var s2: String +} + struct Pair { var t: (a: Int, b: Int) } @@ -2088,3 +2094,60 @@ bb3: %r = tuple () return %r } + +// Just check that LICM doesn't produce invalid SIL. +// CHECK-LABEL: sil [ossa] @partial_load_take_is_not_supported : +// CHECK-LABEL: } // end sil function 'partial_load_take_is_not_supported' +sil [ossa] @partial_load_take_is_not_supported : $@convention(thin) (@inout S2, Int) -> () { +bb0(%0 : $*S2, %1 : $Int): + br bb1 + +bb1: + %4 = struct_element_addr %0, #S2.s1 + %5 = load [take] %4 + %6 = struct_element_addr %0, #S2.s2 + %7 = load [take] %6 + %8 = struct $S2 (%1, %5, %7) + store %8 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + %r = tuple () + return %r +} + +// CHECK-LABEL: sil [ossa] @partial_load_copy : +// CHECK: bb1([[V:%.*]] : @owned $S2): +// CHECK: [[B1:%.*]] = begin_borrow [[V]] +// CHECK-NEXT: [[SE1:%.*]] = struct_extract [[B1]] : $S2, #S2.s1 +// CHECK-NEXT: = copy_value [[SE1]] +// CHECK: [[B2:%.*]] = begin_borrow [[V]] +// CHECK-NEXT: [[SE2:%.*]] = struct_extract [[B2]] : $S2, #S2.s2 +// CHECK-NEXT: = copy_value [[SE2]] +// CHECK-LABEL: } // end sil function 'partial_load_copy' +sil [ossa] @partial_load_copy : $@convention(thin) (@inout S2, Int) -> () { +bb0(%0 : $*S2, %1 : $Int): + br bb1 + +bb1: + %4 = struct_element_addr %0, #S2.s1 + %5 = load [copy] %4 + %6 = struct_element_addr %0, #S2.s2 + %7 = load [copy] %6 + %8 = struct $S2 (%1, %5, %7) + %9 = load [take] %0 + destroy_value %9 + store %8 to [init] %0 + cond_br undef, bb2, bb3 + +bb2: + br bb1 + +bb3: + %r = tuple () + return %r +} +