Skip to content

Commit 0e4dc2f

Browse files
authored
Merge pull request #85068 from eeckstein/fix-licm
LoopInvariantCodeMotion: don't extend "read" access scopes over memory writes to the same address
2 parents c676c29 + 5b8a1a7 commit 0e4dc2f

13 files changed

+129
-71
lines changed

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LoopInvariantCodeMotion.swift

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -296,13 +296,15 @@ private func collectMovableInstructions(
296296
continue
297297
}
298298

299-
if !analyzedInstructions.sideEffectsMayRelease || !analyzedInstructions.loopSideEffectsMayWriteTo(address: fixLifetimeInst.operand.value, context.aliasAnalysis) {
299+
if !analyzedInstructions.sideEffectsMayRelease ||
300+
!analyzedInstructions.sideEffectsMayWrite(to: fixLifetimeInst.operand.value, context.aliasAnalysis)
301+
{
300302
movableInstructions.sinkDown.append(fixLifetimeInst)
301303
}
302304
case let loadInst as LoadInst:
303305
// Avoid quadratic complexity in corner cases. Usually, this limit will not be exceeded.
304306
if loadInstCounter * analyzedInstructions.loopSideEffects.count < 8000,
305-
!analyzedInstructions.loopSideEffectsMayWriteTo(address: loadInst.operand.value, context.aliasAnalysis) {
307+
!analyzedInstructions.sideEffectsMayWrite(to: loadInst.address, context.aliasAnalysis) {
306308
movableInstructions.hoistUp.append(loadInst)
307309
}
308310

@@ -532,13 +534,41 @@ private extension AnalyzedInstructions {
532534

533535
/// Returns true if `loopSideEffects` contains any memory writes which
534536
/// may alias with the memory `address`.
535-
func loopSideEffectsMayWriteTo(address: Value, _ aliasAnalysis: AliasAnalysis) -> Bool {
537+
func sideEffectsMayWrite(to address: Value, _ aliasAnalysis: AliasAnalysis) -> Bool {
536538
return loopSideEffects
537539
.contains { sideEffect in
538540
sideEffect.mayWrite(toAddress: address, aliasAnalysis)
539541
}
540542
}
541-
543+
544+
func sideEffectsMayWrite(to address: Value,
545+
outsideOf scope: InstructionRange,
546+
_ context: FunctionPassContext
547+
) -> Bool {
548+
for sideEffectInst in loopSideEffects {
549+
if sideEffectInst.mayWrite(toAddress: address, context.aliasAnalysis),
550+
!scope.inclusiveRangeContains(sideEffectInst)
551+
{
552+
return true
553+
}
554+
}
555+
return false
556+
}
557+
558+
func sideEffectsMayReadOrWrite(to address: Value,
559+
outsideOf scope: InstructionRange,
560+
_ context: FunctionPassContext
561+
) -> Bool {
562+
for sideEffectInst in loopSideEffects {
563+
if sideEffectInst.mayReadOrWrite(address: address, context.aliasAnalysis),
564+
!scope.inclusiveRangeContains(sideEffectInst)
565+
{
566+
return true
567+
}
568+
}
569+
return false
570+
}
571+
542572
/// Find all loads that contain `accessPath`. Split them into a load with
543573
/// identical `accessPath` and a set of non-overlapping loads. Add the new
544574
/// non-overlapping loads to `loads`.
@@ -1206,47 +1236,19 @@ private extension ScopedInstruction {
12061236
case is BeginApplyInst:
12071237
return true // Has already been checked with other full applies.
12081238
case let loadBorrowInst as LoadBorrowInst:
1209-
for sideEffectInst in analyzedInstructions.loopSideEffects {
1210-
if let endBorrow = sideEffectInst as? EndBorrowInst,
1211-
let begin = endBorrow.borrow as? LoadBorrowInst,
1212-
begin == self
1213-
{
1214-
continue
1215-
}
1216-
if sideEffectInst.mayWrite(toAddress: loadBorrowInst.address, context.aliasAnalysis),
1217-
!scope.contains(sideEffectInst)
1218-
{
1219-
return false
1220-
}
1221-
}
1222-
return true
1239+
return !analyzedInstructions.sideEffectsMayWrite(to: loadBorrowInst.address, outsideOf: scope, context)
12231240

12241241
case let beginAccess as BeginAccessInst:
1225-
for fullApplyInst in analyzedInstructions.fullApplies {
1226-
guard mayWriteToMemory && fullApplyInst.mayReadOrWrite(address: beginAccess.address, context.aliasAnalysis) ||
1227-
!mayWriteToMemory && fullApplyInst.mayWrite(toAddress: beginAccess.address, context.aliasAnalysis) else {
1228-
continue
1229-
}
1230-
1231-
// After hoisting the begin/end_access the apply will be within the scope, so it must not have a conflicting access.
1232-
if !scope.contains(fullApplyInst) {
1233-
return false
1234-
}
1235-
}
1236-
1237-
switch beginAccess.address.accessPath.base {
1238-
case .class, .global:
1239-
for sideEffect in analyzedInstructions.loopSideEffects where sideEffect.mayRelease {
1240-
// Since a class might have a deinitializer, hoisting begin/end_access pair could violate
1241-
// exclusive access if the deinitializer accesses address used by begin_access.
1242-
if !scope.contains(sideEffect) {
1243-
return false
1244-
}
1245-
}
1246-
1247-
return true
1248-
default:
1249-
return true
1242+
if beginAccess.accessKind == .read {
1243+
// Check that we don't generate nested accesses when extending the access scope. Also, we must not
1244+
// extend a "read" access scope over a memory write (to the same address) even if the write is _not_
1245+
// in an access scope, because this would confuse alias analysis.
1246+
return !analyzedInstructions.sideEffectsMayWrite(to: beginAccess.address, outsideOf: scope, context)
1247+
} else {
1248+
// This does not include memory-reading instructions which are not in `loopSideEffects`, like a
1249+
// plain `load`. This is fine because we can extend a "modify" access scope over memory reads
1250+
// (of the same address) as long as we are not generating nested accesses.
1251+
return !analyzedInstructions.sideEffectsMayReadOrWrite(to: beginAccess.address, outsideOf: scope, context)
12501252
}
12511253
case is BeginBorrowInst, is StoreBorrowInst:
12521254
// Ensure the value is produced outside the loop.

SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,33 @@ extension LoadBorrowInst : VerifiableInstruction {
154154
}
155155
}
156156

157+
extension BeginAccessInst : VerifiableInstruction {
158+
func verify(_ context: VerifierContext) {
159+
if context.silStage == .raw {
160+
return
161+
}
162+
// Catch violations like
163+
// ```
164+
// %1 = begin_access [read] %0
165+
// store %2 to %0
166+
// end_access %1
167+
// ```
168+
if address.type.isMoveOnly && enforcement == .static {
169+
// This is a workaround for a bug in the move-only checker: rdar://151841926.
170+
// The move-only checker sometimes inserts destroy_addr within read-only static access scopes.
171+
// TODO: remove this once the bug is fixed.
172+
return
173+
}
174+
if accessKind == .read {
175+
var mutatingInstructions = MutatingUsesWalker(context)
176+
defer { mutatingInstructions.deinitialize() }
177+
178+
mutatingInstructions.findMutatingUses(of: self.address)
179+
mutatingInstructions.verifyNoMutatingUsesInLinearLiverange(of: self)
180+
}
181+
}
182+
}
183+
157184
extension VectorBaseAddrInst : VerifiableInstruction {
158185
func verify(_ context: VerifierContext) {
159186
require(vector.type.isBuiltinFixedArray,
@@ -210,9 +237,7 @@ private struct MutatingUsesWalker : AddressDefUseWalker {
210237
}
211238
}
212239

213-
private mutating func verifyNoMutatingUsesInLinearLiverange(of startInst: SingleValueInstruction) {
214-
assert(startInst is LoadBorrowInst || startInst is BorrowedFromInst)
215-
240+
mutating func verifyNoMutatingUsesInLinearLiverange(of startInst: SingleValueInstruction) {
216241
var instWorklist = InstructionWorklist(context)
217242
defer { instWorklist.deinitialize() }
218243

@@ -223,7 +248,7 @@ private struct MutatingUsesWalker : AddressDefUseWalker {
223248

224249
while let inst = instWorklist.pop() {
225250
require(!mutatingInstructions.contains(inst),
226-
"Load borrow invalidated by a local write", atInstruction: inst)
251+
"read-only scope invalidated by a local write", atInstruction: inst)
227252
instWorklist.pushPredecessors(of: inst, ignoring: startInst)
228253
}
229254
}

test/SIL/OwnershipVerifier/load_borrow_invalidation_partial_apply.sil

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ bb0(%0 : @owned $WrapperStruct):
5555
// we run sil-opt with -dont-abort-on-memory-lifetime-error.
5656

5757
// CHECK-LABEL: Begin Error in function caller2
58-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
58+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
5959
// CHECK-LABEL: End Error in function caller2
6060
sil [ossa] @caller2 : $@convention(thin) (@owned WrapperStruct) -> () {
6161
bb0(%0 : @owned $WrapperStruct):

test/SIL/OwnershipVerifier/load_borrow_verify_errors.sil

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ sil @use_guaranteed : $@convention(thin) (@guaranteed Klass) -> ()
88

99
// Write: store {{.*}} [assign] {{.*}}
1010
// CHECK: Begin Error in function test_write_reborrow
11-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
11+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
1212
// CHECK: End Error in function test_write_reborrow
1313
sil [ossa] @test_write_reborrow : $@convention(thin) (@owned Klass, @owned Klass) -> () {
1414
bb0(%0 : @owned $Klass, %1 : @owned $Klass):
@@ -30,7 +30,7 @@ bb2(%ldarg : @guaranteed $Klass):
3030
}
3131

3232
// CHECK: Begin Error in function test_multiple_loadborrows
33-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
33+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
3434
// CHECK: Verifying instruction:
3535
// CHECK: -> destroy_addr
3636
// CHECK: End Error in function test_multiple_loadborrows
@@ -84,7 +84,7 @@ struct MyStruct {
8484
}
8585

8686
// CHECK: Begin Error in function test_is_unique
87-
// CHECK: SIL verification failed: Load borrow invalidated by a local write
87+
// CHECK: SIL verification failed: read-only scope invalidated by a local write
8888
// CHECK: -> %4 = is_unique %1 : $*ArrayIntBuffer
8989
// CHECK: End Error in function test_is_unique
9090
sil [ossa] @test_is_unique : $@convention(thin) (@in MyArray<MyStruct>) -> () {

test/SIL/memory_lifetime.sil

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ bb0(%0 : @owned $T, %1 : @owned $Inner):
825825
%4 = alloc_stack $Inner
826826
store %1 to [init] %4
827827
%6 = mark_dependence %4 on %2
828-
%7 = begin_access [read] [dynamic] %6
828+
%7 = begin_access [modify] [dynamic] %6
829829
%8 = load [take] %7
830830
end_access %7
831831
dealloc_stack %4
@@ -841,7 +841,7 @@ bb0(%0 : @owned $T, %1 : @owned $Inner):
841841
%4 = alloc_stack $Inner
842842
store %1 to [init] %4
843843
mark_dependence_addr %4 on %2
844-
%7 = begin_access [read] [dynamic] %4
844+
%7 = begin_access [modify] [dynamic] %4
845845
%8 = load [take] %7
846846
end_access %7
847847
dealloc_stack %4

test/SILOptimizer/access_enforcement_opts.sil

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1673,12 +1673,12 @@ bb0:
16731673
// and the caller storage is an Argument at a different position.
16741674
//
16751675
// CHECK-LABEL: sil @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
1676-
// CHECK: begin_access [read] [dynamic] [no_nested_conflict]
1676+
// CHECK: begin_access [modify] [dynamic] [no_nested_conflict]
16771677
// CHECK-LABEL: } // end sil function 'testTransformArgumentCaller'
16781678
sil @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
16791679
bb0(%0 : $Int64, %1 : ${ var Int64 }, %2 : ${ var Int64 }):
16801680
%boxadr = project_box %1 : $ { var Int64 }, 0
1681-
%access = begin_access [read] [dynamic] [no_nested_conflict] %boxadr : $*Int64
1681+
%access = begin_access [modify] [dynamic] [no_nested_conflict] %boxadr : $*Int64
16821682
%f = function_ref @testTransformArgumentCallee : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
16831683
%call = apply %f(%2) : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
16841684
store %0 to %access : $*Int64

test/SILOptimizer/access_enforcement_opts_ossa.sil

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1757,12 +1757,12 @@ bb0:
17571757
// and the caller storage is an Argument at a different position.
17581758
//
17591759
// CHECK-LABEL: sil [ossa] @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
1760-
// CHECK: begin_access [read] [dynamic] [no_nested_conflict]
1760+
// CHECK: begin_access [modify] [dynamic] [no_nested_conflict]
17611761
// CHECK-LABEL: } // end sil function 'testTransformArgumentCaller'
17621762
sil [ossa] @testTransformArgumentCaller : $@convention(thin) (Int64, @guaranteed { var Int64 }, @guaranteed { var Int64 }) -> Int64 {
17631763
bb0(%0 : $Int64, %1 : @guaranteed ${ var Int64 }, %2 : @guaranteed ${ var Int64 }):
17641764
%boxadr = project_box %1 : $ { var Int64 }, 0
1765-
%access = begin_access [read] [dynamic] [no_nested_conflict] %boxadr : $*Int64
1765+
%access = begin_access [modify] [dynamic] [no_nested_conflict] %boxadr : $*Int64
17661766
%f = function_ref @testTransformArgumentCallee : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
17671767
%call = apply %f(%2) : $@convention(thin) (@guaranteed { var Int64 }) -> Int64
17681768
store %0 to [trivial] %access : $*Int64

test/SILOptimizer/access_storage_analysis.sil

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,12 @@ bb0(%0 : ${ var Int }, %1 : $Int):
103103
}
104104

105105
// CHECK-LABEL: @writeIdentifiedBox
106-
// CHECK: [read] Box %1 = alloc_box ${ var Int }
106+
// CHECK: [modify] Box %1 = alloc_box ${ var Int }
107107
sil @writeIdentifiedBox : $@convention(thin) (Int) -> () {
108108
bb0(%0 : $Int):
109109
%1 = alloc_box ${ var Int }
110110
%2 = project_box %1 : ${ var Int }, 0
111-
%3 = begin_access [read] [dynamic] %2 : $*Int
111+
%3 = begin_access [modify] [dynamic] %2 : $*Int
112112
store %0 to %3 : $*Int
113113
end_access %3 : $*Int
114114
%6 = tuple ()
@@ -398,7 +398,7 @@ bb0(%0 : $C):
398398
sil @writeIdentifiedNestedClass : $@convention(thin) (@guaranteed C, Int) -> () {
399399
bb0(%0 : $C, %1 : $Int):
400400
%2 = ref_element_addr %0 : $C, #C.property
401-
%3 = begin_access [read] [dynamic] %2 : $*Int
401+
%3 = begin_access [modify] [dynamic] %2 : $*Int
402402
%4 = begin_access [modify] [dynamic] %3 : $*Int
403403
store %1 to %4 : $*Int
404404
end_access %4 : $*Int

test/SILOptimizer/access_storage_analysis_ossa.sil

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,12 @@ bb0(%0 : @guaranteed ${ var Int }, %1 : $Int):
103103
}
104104

105105
// CHECK-LABEL: @writeIdentifiedBox
106-
// CHECK: [read] Box %1 = alloc_box ${ var Int }
106+
// CHECK: [modify] Box %1 = alloc_box ${ var Int }
107107
sil [ossa] @writeIdentifiedBox : $@convention(thin) (Int) -> () {
108108
bb0(%0 : $Int):
109109
%1 = alloc_box ${ var Int }
110110
%2 = project_box %1 : ${ var Int }, 0
111-
%3 = begin_access [read] [dynamic] %2 : $*Int
111+
%3 = begin_access [modify] [dynamic] %2 : $*Int
112112
store %0 to [trivial] %3 : $*Int
113113
end_access %3 : $*Int
114114
destroy_value %1 : ${ var Int }
@@ -409,7 +409,7 @@ sil [ossa] @writeIdentifiedNestedClass : $@convention(thin) (@guaranteed C, Int)
409409
bb0(%0 : @guaranteed $C, %1 : $Int):
410410
%borrow = begin_borrow %0 : $C
411411
%2 = ref_element_addr %borrow : $C, #C.property
412-
%3 = begin_access [read] [dynamic] %2 : $*Int
412+
%3 = begin_access [modify] [dynamic] %2 : $*Int
413413
%4 = begin_access [modify] [dynamic] %3 : $*Int
414414
store %1 to [trivial] %4 : $*Int
415415
end_access %4 : $*Int

test/SILOptimizer/copy_propagation.sil

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ bb0:
471471
// def
472472
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
473473
%copy = copy_value %def : $AnyObject
474-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
474+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
475475
// use
476476
store %def to [init] %adr : $*AnyObject
477477
%obj = load [copy] %access : $*AnyObject
@@ -519,7 +519,7 @@ bb0:
519519
// def
520520
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
521521
%copy = copy_value %def : $AnyObject
522-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
522+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
523523
// use
524524
store %def to [init] %adr : $*AnyObject
525525
br bb1
@@ -561,7 +561,7 @@ sil [ossa] @testFullOverlapInDefBlock : $@convention(thin) () -> () {
561561
bb0:
562562
%box = alloc_box ${ var AnyObject }, var, name "x"
563563
%adr = project_box %box : ${ var AnyObject }, 0
564-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
564+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
565565
%f = function_ref @getObject : $@convention(thin) () -> @owned AnyObject
566566
// def
567567
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
@@ -609,7 +609,7 @@ sil [ossa] @testFullOverlapBeforeDefBlock : $@convention(thin) () -> () {
609609
bb0:
610610
%box = alloc_box ${ var AnyObject }, var, name "x"
611611
%adr = project_box %box : ${ var AnyObject }, 0
612-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
612+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
613613
br bb1
614614

615615
bb1:
@@ -657,7 +657,7 @@ bb0:
657657
// def
658658
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
659659
%copy = copy_value %def : $AnyObject
660-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
660+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
661661
// use
662662
store %def to [init] %adr : $*AnyObject
663663
destroy_value %copy : $AnyObject
@@ -692,7 +692,7 @@ bb0:
692692
// def
693693
%def = apply %f() : $@convention(thin) () -> @owned AnyObject
694694
%copy = copy_value %def : $AnyObject
695-
%access = begin_access [read] [dynamic] %adr : $*AnyObject
695+
%access = begin_access [modify] [dynamic] %adr : $*AnyObject
696696
// use
697697
store %def to [init] %adr : $*AnyObject
698698
br bb1

0 commit comments

Comments
 (0)