From df2401919517979abf167e42794dc0da5b9bf882 Mon Sep 17 00:00:00 2001 From: Jakub Florek Date: Wed, 10 Sep 2025 16:12:05 +0100 Subject: [PATCH 1/3] Rename Cloner.cloneRecursivelyToGlobal. --- .../FunctionPasses/InitializeStaticGlobals.swift | 2 +- .../Sources/Optimizer/FunctionPasses/ObjectOutliner.swift | 6 +++--- .../InstructionSimplification/SimplifyLoad.swift | 2 +- SwiftCompilerSources/Sources/SIL/Utilities/Cloner.swift | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/InitializeStaticGlobals.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/InitializeStaticGlobals.swift index 6f181f1339d95..272ec5ae84cf5 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/InitializeStaticGlobals.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/InitializeStaticGlobals.swift @@ -289,7 +289,7 @@ private indirect enum GlobalInitValue { fatalError("cannot materialize undefined init value") case .constant(let value): - return cloner.cloneRecursivelyToGlobal(value: value) + return cloner.cloneRecursively(globalInitValue: value) case .aggregate(let fields): if type.isStruct { diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ObjectOutliner.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ObjectOutliner.swift index b141812d29bc4..f0a41bc89ce4f 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ObjectOutliner.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/ObjectOutliner.swift @@ -370,7 +370,7 @@ private func constructObject(of allocRef: AllocRefInstBase, // Create the initializers for the fields var objectArgs = [Value]() for store in storesToClassFields { - objectArgs.append(cloner.cloneRecursivelyToGlobal(value: store.source as! SingleValueInstruction)) + objectArgs.append(cloner.cloneRecursively(globalInitValue: store.source as! SingleValueInstruction)) } let globalBuilder = Builder(staticInitializerOf: global, context) @@ -382,7 +382,7 @@ private func constructObject(of allocRef: AllocRefInstBase, for elementIdx in 0.. { } return cloned } - - public mutating func cloneRecursivelyToGlobal(value: Value) -> Value { - guard let cloned = cloneRecursively(value: value, customGetCloned: { value, cloner in + + public mutating func cloneRecursively(globalInitValue: Value) -> Value { + guard let cloned = cloneRecursively(value: globalInitValue, customGetCloned: { value, cloner in guard let beginAccess = value as? BeginAccessInst else { return .defaultValue } // Skip access instructions, which might be generated for UnsafePointer globals which point to other globals. - let clonedOperand = cloner.cloneRecursivelyToGlobal(value: beginAccess.address) + let clonedOperand = cloner.cloneRecursively(globalInitValue: beginAccess.address) cloner.recordFoldedValue(beginAccess, mappedTo: clonedOperand) return .customValue(clonedOperand) }) else { From 0b75a81b65f8e2edda98451ef6456e923423e0ba Mon Sep 17 00:00:00 2001 From: Jakub Florek Date: Wed, 10 Sep 2025 16:12:34 +0100 Subject: [PATCH 2/3] Add licm Ownership support. --- .../LoopInvariantCodeMotion.swift | 315 ++++++++++-------- test/SILOptimizer/licm.sil | 96 +++++- 2 files changed, 267 insertions(+), 144 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift index 2c1d53e763393..dc041f326c1f5 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift @@ -17,6 +17,10 @@ let loopInvariantCodeMotionPass = FunctionPass(name: "loop-invariant-code-motion for loop in context.loopTree.loops { optimizeTopLevelLoop(topLevelLoop: loop, context) } + + if context.needFixStackNesting { + context.fixStackNesting(in: function) + } } private func optimizeTopLevelLoop(topLevelLoop: Loop, _ context: FunctionPassContext) { @@ -57,7 +61,7 @@ private struct MovableInstructions { var loadsAndStores: [Instruction] = [] var hoistUp: [Instruction] = [] var sinkDown: [Instruction] = [] - var scopedInsts: [Instruction] = [] + var scopedInsts: [ScopedInstruction] = [] } /// Analyzed instructions inside the currently processed loop. @@ -80,10 +84,10 @@ private struct AnalyzedInstructions { /// * an apply to the addressor of the global /// * a builtin "once" of the global initializer var globalInitCalls: Stack - var readOnlyApplies: Stack + var readOnlyApplies: Stack var loads: Stack var stores: Stack - var beginAccesses: Stack + var scopedInsts: Stack var fullApplies: Stack /// `true` if the loop has instructions which (may) read from memory, which are not in `Loads` and not in `sideEffects`. @@ -97,10 +101,10 @@ private struct AnalyzedInstructions { self.blockSideEffectBottomMarker = loopSideEffects.top self.globalInitCalls = Stack(context) - self.readOnlyApplies = Stack(context) + self.readOnlyApplies = Stack(context) self.loads = Stack(context) self.stores = Stack(context) - self.beginAccesses = Stack(context) + self.scopedInsts = Stack(context) self.fullApplies = Stack(context) } @@ -110,7 +114,7 @@ private struct AnalyzedInstructions { loopSideEffects.deinitialize() loads.deinitialize() stores.deinitialize() - beginAccesses.deinitialize() + scopedInsts.deinitialize() fullApplies.deinitialize() } @@ -134,8 +138,6 @@ private func analyzeLoopAndSplitLoads(loop: Loop, _ context: FunctionPassContext analyzeInstructions(in: loop, &analyzedInstructions, &movableInstructions, context) - collectHoistableReadOnlyApplies(analyzedInstructions, &movableInstructions, context) - collectHoistableGlobalInitCalls(in: loop, analyzedInstructions, &movableInstructions, context) collectProjectableAccessPathsAndSplitLoads(in: loop, &analyzedInstructions, &movableInstructions, context) @@ -158,60 +160,46 @@ private func analyzeInstructions( analyzedInstructions.markBeginOfBlock() for inst in bb.instructions { - // TODO: Remove once support for values with ownership implemented. - if inst.hasOwnershipOperandsOrResults { - analyzedInstructions.analyzeSideEffects(ofInst: inst) - - // Collect fullApplies to be checked in analyzeBeginAccess - if let fullApply = inst as? FullApplySite { - analyzedInstructions.fullApplies.append(fullApply) - } - - continue - } - switch inst { case is FixLifetimeInst: break // We can ignore the side effects of FixLifetimes case let loadInst as LoadInst: analyzedInstructions.loads.append(loadInst) + case let uncheckedOwnershipConversionInst as UncheckedOwnershipConversionInst: + analyzedInstructions.analyzeSideEffects(ofInst: uncheckedOwnershipConversionInst) case let storeInst as StoreInst: - switch storeInst.storeOwnership { - case .assign, .initialize: - continue // TODO: Add support - case .unqualified, .trivial: - break - } analyzedInstructions.stores.append(storeInst) analyzedInstructions.analyzeSideEffects(ofInst: storeInst) case let beginAccessInst as BeginAccessInst: - analyzedInstructions.beginAccesses.append(beginAccessInst) + analyzedInstructions.scopedInsts.append(beginAccessInst) analyzedInstructions.analyzeSideEffects(ofInst: beginAccessInst) + case let beginBorrowInst as BeginBorrowInstruction: + analyzedInstructions.analyzeSideEffects(ofInst: beginBorrowInst) case let refElementAddrInst as RefElementAddrInst: movableInstructions.speculativelyHoistable.append(refElementAddrInst) case let condFailInst as CondFailInst: analyzedInstructions.analyzeSideEffects(ofInst: condFailInst) - case let applyInst as ApplyInst: - if applyInst.isSafeReadOnlyApply(context.calleeAnalysis) { - analyzedInstructions.readOnlyApplies.append(applyInst) - } else if let callee = applyInst.referencedFunction, + case let fullApply as FullApplySite: + if fullApply.isSafeReadOnlyApply(context.calleeAnalysis) { + analyzedInstructions.readOnlyApplies.append(fullApply) + } else if let callee = fullApply.referencedFunction, callee.isGlobalInitFunction, // Calls to global inits are different because we don't care about side effects which are "after" the call in the loop. - !applyInst.globalInitMayConflictWith( + !fullApply.globalInitMayConflictWith( blockSideEffectSegment: analyzedInstructions.sideEffectsOfCurrentBlock, context.aliasAnalysis ) { // Check against side-effects within the same block. // Side-effects in other blocks are checked later (after we // scanned all blocks of the loop) in `collectHoistableGlobalInitCalls`. - analyzedInstructions.globalInitCalls.append(applyInst) + analyzedInstructions.globalInitCalls.append(fullApply) } + analyzedInstructions.fullApplies.append(fullApply) + // Check for array semantics and side effects - same as default fallthrough default: switch inst { - case let fullApply as FullApplySite: - analyzedInstructions.fullApplies.append(fullApply) case let builtinInst as BuiltinInst: switch builtinInst.id { case .Once, .OnceWithContext: @@ -236,27 +224,6 @@ private func analyzeInstructions( } } -/// Process collected read only applies. Moves them to `hoistUp` if they don't conflict with any side effects. -private func collectHoistableReadOnlyApplies( - _ analyzedInstructions: AnalyzedInstructions, - _ movableInstructions: inout MovableInstructions, - _ context: FunctionPassContext -) { - var counter = 0 - for readOnlyApply in analyzedInstructions.readOnlyApplies { - // Avoid quadratic complexity in corner cases. Usually, this limit will not be exceeded. - if counter * analyzedInstructions.loopSideEffects.count < 8000, - readOnlyApply.isSafeReadOnlyApply( - for: analyzedInstructions.loopSideEffects, - context.aliasAnalysis, - context.calleeAnalysis - ) { - movableInstructions.hoistUp.append(readOnlyApply) - } - counter += 1 - } -} - /// Process collected global init calls. Moves them to `hoistUp` if they don't conflict with any side effects. private func collectHoistableGlobalInitCalls( in loop: Loop, @@ -320,13 +287,9 @@ private func collectMovableInstructions( _ context: FunctionPassContext ) { var loadInstCounter = 0 + var readOnlyApplyCounter = 0 for bb in loop.loopBlocks { for inst in bb.instructions { - // TODO: Remove once support for values with ownership implemented. - if inst.hasOwnershipOperandsOrResults { - continue - } - switch inst { case let fixLifetimeInst as FixLifetimeInst: guard fixLifetimeInst.parentBlock.dominates(loop.preheader!, context.dominatorTree) else { @@ -339,19 +302,20 @@ private func collectMovableInstructions( case let loadInst as LoadInst: // Avoid quadratic complexity in corner cases. Usually, this limit will not be exceeded. if loadInstCounter * analyzedInstructions.loopSideEffects.count < 8000, - !analyzedInstructions.loopSideEffectsMayWriteTo(address: loadInst.operand.value, context.aliasAnalysis), - loadInst.loadOwnership != .take, loadInst.loadOwnership != .copy { // TODO: Add support for take and copy loads. + !analyzedInstructions.loopSideEffectsMayWriteTo(address: loadInst.operand.value, context.aliasAnalysis) { movableInstructions.hoistUp.append(loadInst) } loadInstCounter += 1 movableInstructions.loadsAndStores.append(loadInst) + case is UncheckedOwnershipConversionInst: + break // TODO: Add support case let storeInst as StoreInst: switch storeInst.storeOwnership { - case .assign, .initialize: + case .assign: continue // TODO: Add support - case .unqualified, .trivial: + case .unqualified, .trivial, .initialize: break } movableInstructions.loadsAndStores.append(storeInst) @@ -362,9 +326,33 @@ private func collectMovableInstructions( // must - after hoisting - also be executed before said access. movableInstructions.hoistUp.append(condFailInst) case let beginAccessInst as BeginAccessInst: - if beginAccessInst.canBeHoisted(outOf: loop, analyzedInstructions: analyzedInstructions, context) { + if beginAccessInst.canScopedInstructionBeHoisted(outOf: loop, analyzedInstructions: analyzedInstructions, context) { movableInstructions.scopedInsts.append(beginAccessInst) } + case let beginBorrowInst as BeginBorrowInstruction: + if !beginBorrowInst.isLexical && beginBorrowInst.canScopedInstructionBeHoisted(outOf: loop, analyzedInstructions: analyzedInstructions, context) { + movableInstructions.scopedInsts.append(beginBorrowInst) + } + case let fullApplySite as FullApplySite: + guard analyzedInstructions.readOnlyApplies.contains(where: { $0 == fullApplySite }) else { + break + } + + // Avoid quadratic complexity in corner cases. Usually, this limit will not be exceeded. + if readOnlyApplyCounter * analyzedInstructions.loopSideEffects.count < 8000, + fullApplySite.isSafeReadOnlyApply( + for: analyzedInstructions.loopSideEffects, + context.aliasAnalysis, + context.calleeAnalysis + ) { + if let beginApplyInst = fullApplySite as? BeginApplyInst { + movableInstructions.scopedInsts.append(beginApplyInst) + } else { + movableInstructions.hoistUp.append(fullApplySite) + } + + readOnlyApplyCounter += 1 + } default: break } @@ -433,7 +421,7 @@ private extension AnalyzedInstructions { if (loopSideEffects.contains { sideEffect in switch sideEffect { case let storeInst as StoreInst: - if storeInst.isNonInitializingStoreTo(accessPath) { + if storeInst.storesTo(accessPath) { return false } case let loadInst as LoadInst: @@ -450,13 +438,13 @@ private extension AnalyzedInstructions { } if (loads.contains { loadInst in - loadInst.mayRead(fromAddress: storeAddr, aliasAnalysis) && loadInst.loadOwnership != .take && !loadInst.overlaps(accessPath: accessPath) + loadInst.mayRead(fromAddress: storeAddr, aliasAnalysis) && !loadInst.overlaps(accessPath: accessPath) }) { return false } if (stores.contains { storeInst in - storeInst.mayWrite(toAddress: storeAddr, aliasAnalysis) && !storeInst.isNonInitializingStoreTo(accessPath) + storeInst.mayWrite(toAddress: storeAddr, aliasAnalysis) && !storeInst.storesTo(accessPath) }) { return false } @@ -661,18 +649,29 @@ private extension MovableInstructions { var changed = false - for specialInst in scopedInsts { - guard let beginAccessInst = specialInst as? BeginAccessInst else { - continue + for scopedInst in scopedInsts { + if let storeBorrowInst = scopedInst as? StoreBorrowInst { + _ = storeBorrowInst.allocStack.hoist(outOf: loop, context) + + var sankFirst = false + for deallocStack in storeBorrowInst.allocStack.deallocations { + if sankFirst { + context.erase(instruction: deallocStack) + } else { + sankFirst = deallocStack.sink(outOf: loop, context) + } + } + + context.notifyInvalidatedStackNesting() } - guard specialInst.hoist(outOf: loop, context) else { + guard scopedInst.hoist(outOf: loop, context) else { continue } // We only want to sink the first end_access and erase the rest to not introduce duplicates. var sankFirst = false - for endAccess in beginAccessInst.endAccessInstructions { + for endAccess in scopedInst.endInstructions { if sankFirst { context.erase(instruction: endAccess) } else { @@ -693,10 +692,10 @@ private extension MovableInstructions { ) -> Bool { // Initially load the value in the loop pre header. let builder = Builder(before: loop.preheader!.terminator, context) - var storeAddr: Value? + var firstStore: StoreInst? // If there are multiple stores in a block, only the last one counts. - for case let storeInst as StoreInst in loadsAndStores where storeInst.isNonInitializingStoreTo(accessPath) { + for case let storeInst as StoreInst in loadsAndStores where storeInst.storesTo(accessPath) { // If a store just stores the loaded value, bail. The operand (= the load) // will be removed later, so it cannot be used as available value. // This corner case is surprisingly hard to handle, so we just give up. @@ -705,9 +704,9 @@ private extension MovableInstructions { return false } - if storeAddr == nil { - storeAddr = storeInst.destination - } else if storeInst.destination.type != storeAddr!.type { + if firstStore == nil { + firstStore = storeInst + } else if storeInst.destination.type != firstStore!.destination.type { // This transformation assumes that the values of all stores in the loop // must be interchangeable. It won't work if stores different types // because of casting or payload extraction even though they have the @@ -716,26 +715,26 @@ private extension MovableInstructions { } } - guard let storeAddr else { + guard let firstStore else { return false } var ssaUpdater = SSAUpdater( - function: storeAddr.parentFunction, - type: storeAddr.type.objectType, - ownership: .none, + function: firstStore.parentFunction, + type: firstStore.destination.type.objectType, + ownership: firstStore.source.ownership, context ) // Set all stored values as available values in the ssaUpdater. - for case let storeInst as StoreInst in loadsAndStores where storeInst.isNonInitializingStoreTo(accessPath) { + for case let storeInst as StoreInst in loadsAndStores where storeInst.storesTo(accessPath) { ssaUpdater.addAvailableValue(storeInst.source, in: storeInst.parentBlock) } var cloner = Cloner(cloneBefore: loop.preheader!.terminator, context) defer { cloner.deinitialize() } - guard let initialAddr = (cloner.cloneRecursively(value: storeAddr) { srcAddr, cloner in + guard let initialAddr = (cloner.cloneRecursively(value: firstStore.destination) { srcAddr, cloner in switch srcAddr { case is AllocStackInst, is BeginBorrowInst, is MarkDependenceInst: return .stopCloning @@ -754,7 +753,7 @@ private extension MovableInstructions { return false } - let ownership: LoadInst.LoadOwnership = loop.preheader!.terminator.parentFunction.hasOwnership ? .trivial : .unqualified + let ownership: LoadInst.LoadOwnership = firstStore.parentFunction.hasOwnership ? (firstStore.storeOwnership == .initialize ? .take : .trivial) : .unqualified let initialLoad = builder.createLoad(fromAddress: initialAddr, ownership: ownership) ssaUpdater.addAvailableValue(initialLoad, in: loop.preheader!) @@ -774,7 +773,7 @@ private extension MovableInstructions { currentVal = nil } - if let storeInst = inst as? StoreInst, storeInst.isNonInitializingStoreTo(accessPath) { + if let storeInst = inst as? StoreInst, storeInst.storesTo(accessPath) { currentVal = storeInst.source context.erase(instruction: storeInst) changed = true @@ -814,11 +813,10 @@ private extension MovableInstructions { let builder = Builder(before: exitBlock.instructions.first!, context) - let ownership: StoreInst.StoreOwnership = exitBlock.instructions.first!.parentFunction.hasOwnership ? .trivial : .unqualified builder.createStore( source: ssaUpdater.getValue(inMiddleOf: exitBlock), destination: initialAddr, - ownership: ownership + ownership: firstStore.storeOwnership ) changed = true } @@ -833,13 +831,6 @@ private extension MovableInstructions { } private extension Instruction { - var hasOwnershipOperandsOrResults: Bool { - guard parentFunction.hasOwnership else { return false } - - return results.contains(where: { $0.ownership != .none }) - || operands.contains(where: { $0.value.ownership != .none }) - } - /// 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. func canBeHoisted(outOf loop: Loop, _ context: FunctionPassContext) -> Bool { @@ -861,7 +852,8 @@ private extension Instruction { break } - if memoryEffects == .noEffects { + if memoryEffects == .noEffects, + !results.contains(where: { $0.ownership == .owned }) { return true } @@ -877,11 +869,16 @@ private extension Instruction { if canHoistArraySemanticsCall(to: terminator, context) { hoistArraySemanticsCall(before: terminator, context) } else { - move(before: terminator, context) + if let loadCopyInst = self as? LoadInst, loadCopyInst.loadOwnership == .copy { + hoist(loadCopyInst: loadCopyInst, outOf: loop, context) + return true + } else { + move(before: terminator, context) + } } if let singleValueInst = self as? SingleValueInstruction, - !(self is BeginAccessInst), + !(self is ScopedInstruction || self is AllocStackInst), let identicalInst = (loop.preheader!.instructions.first { otherInst in return singleValueInst != otherInst && singleValueInst.isIdenticalTo(otherInst) }) { @@ -895,11 +892,30 @@ private extension Instruction { return true } + private func hoist(loadCopyInst: LoadInst, outOf loop: Loop, _ context: FunctionPassContext) { + if loop.hasNoExitBlocks { + return + } + + let preheaderBuilder = Builder(before: loop.preheader!.terminator, context) + let preheaderLoadBorrow = preheaderBuilder.createLoadBorrow(fromAddress: loadCopyInst.address) + + let headerBuilder = Builder(before: loadCopyInst, context) + let copyValue = headerBuilder.createCopyValue(operand: preheaderLoadBorrow) + loadCopyInst.replace(with: copyValue, context) + + for exitBlock in loop.exitBlocks where !context.deadEndBlocks.isDeadEnd(exitBlock) { + assert(exitBlock.hasSinglePredecessor, "Exiting edge should not be critical.") + + let exitBlockBuilder = Builder(before: exitBlock.instructions.first!, context) + exitBlockBuilder.createEndBorrow(of: preheaderLoadBorrow) + } + } + func sink(outOf loop: Loop, _ context: FunctionPassContext) -> Bool { - let exitBlocks = loop.exitBlocks var changed = false - for exitBlock in exitBlocks { + for exitBlock in loop.exitBlocks where !context.deadEndBlocks.isDeadEnd(exitBlock) { assert(exitBlock.hasSinglePredecessor, "Exiting edge should not be critical.") if changed { @@ -969,11 +985,7 @@ private extension Instruction { private extension StoreInst { /// Returns a `true` if this store is a store to `accessPath`. - func isNonInitializingStoreTo(_ accessPath: AccessPath) -> Bool { - if self.storeOwnership == .initialize { - return false - } - + func storesTo(_ accessPath: AccessPath) -> Bool { return accessPath == self.destination.accessPath } } @@ -991,7 +1003,7 @@ private extension LoadInst { } } -private extension ApplyInst { +private extension FullApplySite { /// Returns `true` if this apply inst could be safely hoisted. func isSafeReadOnlyApply(_ calleeAnalysis: CalleeAnalysis) -> Bool { guard functionConvention.results.allSatisfy({ $0.convention == .unowned }) else { @@ -1040,6 +1052,10 @@ private extension ApplyInst { is KeyPathInst, is DeallocStackInst, is DeallocStackRefInst, is DeallocRefInst: break + case let endApply as EndApplyInst: + if endApply.beginApply != self { + return false + } default: if sideEffect.mayWriteToMemory { return false @@ -1051,54 +1067,81 @@ private extension ApplyInst { } } -private extension BeginAccessInst { +private extension ScopedInstruction { /// Returns `true` if this begin access is safe to hoist. - func canBeHoisted( + func canScopedInstructionBeHoisted( outOf loop: Loop, analyzedInstructions: AnalyzedInstructions, _ context: FunctionPassContext ) -> Bool { - guard endAccessInstructions.allSatisfy({ loop.contains(block: $0.parentBlock)}) else { + guard endInstructions.allSatisfy({ loop.contains(block: $0.parentBlock) && !($0 is TermInst) }) else { return false } - let areBeginAccessesSafe = analyzedInstructions.beginAccesses - .allSatisfy { otherBeginAccessInst in - guard self != otherBeginAccessInst else { return true } + // Instruction specific preconditions + switch self { + case is BeginAccessInst, is LoadBorrowInst: + guard (analyzedInstructions.scopedInsts + .allSatisfy { otherScopedInst in + guard self != otherScopedInst else { return true } - return self.accessPath.isDistinct(from: otherBeginAccessInst.accessPath) + return operands.first!.value.accessPath.isDistinct(from: otherScopedInst.operand.value.accessPath) + }) else { + return false } - - guard areBeginAccessesSafe else { return false } + default: + break + } - var scope = InstructionRange(begin: self, ends: endAccessInstructions, context) + var scope = InstructionRange(begin: self, ends: endInstructions, context) defer { scope.deinitialize() } - - for fullApplyInst in analyzedInstructions.fullApplies { - guard mayWriteToMemory && fullApplyInst.mayReadOrWrite(address: address, context.aliasAnalysis) || - !mayWriteToMemory && fullApplyInst.mayWrite(toAddress: address, context.aliasAnalysis) else { - continue - } - - // After hoisting the begin/end_access the apply will be within the scope, so it must not have a conflicting access. - if !scope.contains(fullApplyInst) { - return false + + // Instruction specific range related conditions + switch self { + case is BeginApplyInst: + return true // Has already been checked with other full applies. + case is LoadBorrowInst: + for storeInst in analyzedInstructions.stores { + if storeInst.mayWrite(toAddress: operands.first!.value, context.aliasAnalysis) { + if !scope.contains(storeInst) { + return false + } + } } - } + + fallthrough + case is BeginAccessInst: + for fullApplyInst in analyzedInstructions.fullApplies { + guard mayWriteToMemory && fullApplyInst.mayReadOrWrite(address: operands.first!.value, context.aliasAnalysis) || + !mayWriteToMemory && fullApplyInst.mayWrite(toAddress: operands.first!.value, context.aliasAnalysis) else { + continue + } - switch accessPath.base { - case .class, .global: - for sideEffect in analyzedInstructions.loopSideEffects where sideEffect.mayRelease { - // Since a class might have a deinitializer, hoisting begin/end_access pair could violate - // exclusive access if the deinitializer accesses address used by begin_access. - if !scope.contains(sideEffect) { + // After hoisting the begin/end_access the apply will be within the scope, so it must not have a conflicting access. + if !scope.contains(fullApplyInst) { return false } } + + switch operands.first!.value.accessPath.base { + case .class, .global: + for sideEffect in analyzedInstructions.loopSideEffects where sideEffect.mayRelease { + // Since a class might have a deinitializer, hoisting begin/end_access pair could violate + // exclusive access if the deinitializer accesses address used by begin_access. + if !scope.contains(sideEffect) { + return false + } + } - return true + return true + default: + return true + } + case is BeginBorrowInst, is StoreBorrowInst: + // Ensure the value is produced outside the loop. + return !loop.contains(block: operands.first!.value.parentBlock) default: - return true + return false } } } diff --git a/test/SILOptimizer/licm.sil b/test/SILOptimizer/licm.sil index a508ed34069ad..72295f249a555 100644 --- a/test/SILOptimizer/licm.sil +++ b/test/SILOptimizer/licm.sil @@ -1615,11 +1615,14 @@ bb2: return %r1 : $() } -// CHECK-LABEL: sil [ossa] @dont_hoist_non_trivial_load : +// CHECK-LABEL: sil [ossa] @hoist_load_copy : +// CHECK: %1 = load_borrow %0 // CHECK: bb1: -// CHECK-NEXT: load [copy] -// CHECK: } // end sil function 'dont_hoist_non_trivial_load' -sil [ossa] @dont_hoist_non_trivial_load : $@convention(thin) (@inout Builtin.NativeObject) -> () { +// CHECK-NEXT: %3 = copy_value %1 +// CHECK: bb3: +// CHECK-NEXT: end_borrow %1 +// CHECK: } // end sil function 'hoist_load_copy' +sil [ossa] @hoist_load_copy : $@convention(thin) (@inout Builtin.NativeObject) -> () { bb0(%0 : $*Builtin.NativeObject): br bb1 bb1: @@ -1634,6 +1637,56 @@ bb3: return %r : $() } +// CHECK-LABEL: sil [ossa] @hoist_load_take_store_init : +// CHECK: %2 = load [take] %0 +// CHECK: bb1(%4 : @owned $S): +// CHECK-NEXT: fix_lifetime %4 +// CHECK: bb3: +// CHECK-NEXT: store %7 to [init] %0 +// CHECK: } // end sil function 'hoist_load_take_store_init' +sil [ossa] @hoist_load_take_store_init : $@convention(thin) (@inout S, @guaranteed S) -> () { +bb0(%0 : $*S, %new_val : @guaranteed $S): + br bb1 +bb1: + %2 = load [take] %0 + fix_lifetime %2 + destroy_value %2 + %copied = copy_value %new_val + store %copied to [init] %0 + cond_br undef, bb2, bb3 +bb2: + br bb1 +bb3: + %r = tuple () + return %r : $() +} + +sil @$foo_yield : $@yield_once @convention(method) (Int) -> @yields Int { +[global: read] +} + +// CHECK-LABEL: sil [ossa] @hoist_begin_apply : $@convention(thin) (Int) -> () { +// CHECK: bb0(%0 : $Int): +// CHECK: function_ref +// CHECK: begin_apply +// CHECK: bb3: +// CHECK: end_apply +// CHECK: } // end sil function 'hoist_begin_apply' +sil [ossa] @hoist_begin_apply : $@convention(thin) (Int) -> () { +bb0(%0 : $Int): + br bb1 +bb1: + %reader = function_ref @$foo_yield : $@yield_once @convention(method) (Int) -> @yields Int + (%value, %token) = begin_apply %reader(%0) : $@yield_once @convention(method) (Int) -> @yields Int + end_apply %token as $() + cond_br undef, bb2, bb3 +bb2: + br bb1 +bb3: + %r = tuple () + return %r : $() +} + // CHECK-LABEL: sil [ossa] @hoist_trivial_load : // CHECK: load [trivial] // CHECK-NEXT: br bb1 @@ -1652,11 +1705,13 @@ bb3: return %r : $() } -// CHECK-LABEL: sil [ossa] @dont_hoist_load_borrow : -// CHECK: bb1: +// CHECK-LABEL: sil [ossa] @hoist_load_borrow : +// CHECK: bb0(%0 : $*Builtin.NativeObject): // CHECK-NEXT: load_borrow -// CHECK: } // end sil function 'dont_hoist_load_borrow' -sil [ossa] @dont_hoist_load_borrow : $@convention(thin) (@inout Builtin.NativeObject) -> () { +// CHECK: bb3: +// CHECK-NEXT: end_borrow +// CHECK: } // end sil function 'hoist_load_borrow' +sil [ossa] @hoist_load_borrow : $@convention(thin) (@inout Builtin.NativeObject) -> () { bb0(%0 : $*Builtin.NativeObject): br bb1 bb1: @@ -1690,6 +1745,31 @@ bb3: return %r : $() } +// CHECK-LABEL: sil [ossa] @hoist_store_borrow : +// CHECK: bb0(%0 : @guaranteed $S): +// CHECK-NEXT: alloc_stack +// CHECK-NEXT: store_borrow +// CHECK: bb3: +// CHECK-NEXT: end_borrow +// CHECK-NEXT: dealloc_stack +// CHECK: } // end sil function 'hoist_store_borrow' +sil [ossa] @hoist_store_borrow : $@convention(thin) (@guaranteed S) -> () { +bb0(%0 : @guaranteed $S): + br bb1 +bb1: + %s = alloc_stack $S + %2 = store_borrow %0 to %s + fix_lifetime %2 + end_borrow %2 + dealloc_stack %s + cond_br undef, bb2, bb3 +bb2: + br bb1 +bb3: + %r = tuple () + return %r : $() +} + sil @foo : $@convention(thin) (@guaranteed { var Int }) -> () // CHECK-LABEL: sil [ossa] @test_begin_access : $@convention(thin) (Int) -> () { From e84bc084f4ce33443f8ba321344f279d3590c951 Mon Sep 17 00:00:00 2001 From: Jakub Florek Date: Mon, 15 Sep 2025 12:42:30 +0100 Subject: [PATCH 3/3] Check for aliasing destroy_addr before hoisting load_borrow - end_borrow pair. --- .../FunctionPasses/LoopInvariantCodeMotion.swift | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift index dc041f326c1f5..53d992792412e 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift @@ -1100,9 +1100,17 @@ private extension ScopedInstruction { switch self { case is BeginApplyInst: return true // Has already been checked with other full applies. - case is LoadBorrowInst: + case let loadBorrowInst as LoadBorrowInst: + for case let destroyAddrInst as DestroyAddrInst in analyzedInstructions.loopSideEffects { + if context.aliasAnalysis.mayAlias(loadBorrowInst.address, destroyAddrInst.destroyedAddress) { + if !scope.contains(destroyAddrInst) { + return false + } + } + } + for storeInst in analyzedInstructions.stores { - if storeInst.mayWrite(toAddress: operands.first!.value, context.aliasAnalysis) { + if storeInst.mayWrite(toAddress: loadBorrowInst.address, context.aliasAnalysis) { if !scope.contains(storeInst) { return false }