Skip to content

Commit a312732

Browse files
eecksteineeckstein
committed
AliasAnalysis: consider memory effects of a consume/destroy of a class on it's let-fields
Although a let-field can never be mutated, a release or consume of the class must be considered as writing to such a field. This change removes the special handling of let-fields in two places, where they don't belong. Class fields are handled by ImmutableScope anyway. Handling of global let-variable is temporarily removed by this commit. Fixes a miscompile. rdar://142996449
1 parent 600dcd0 commit a312732

File tree

5 files changed

+103
-88
lines changed

5 files changed

+103
-88
lines changed

‎SwiftCompilerSources/Sources/Optimizer/Analysis/AliasAnalysis.swift‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -378,15 +378,6 @@ struct AliasAnalysis {
378378
// The address has unknown escapes. So we have to take the global effects of the called function(s).
379379
memoryEffects = calleeAnalysis.getSideEffects(ofApply: apply).memory
380380
}
381-
// Do some magic for `let` variables. Function calls cannot modify let variables.
382-
// The only exception is that the let variable is directly passed to an indirect out of the apply.
383-
// TODO: make this a more formal and verified approach.
384-
if memoryEffects.write {
385-
let accessBase = memLoc.address.accessBase
386-
if accessBase.isLet && !accessBase.isIndirectResult(of: apply) {
387-
return SideEffects.Memory(read: memoryEffects.read, write: false)
388-
}
389-
}
390381
return memoryEffects
391382
}
392383

@@ -440,11 +431,7 @@ struct AliasAnalysis {
440431
initialWalkingDirection: memLoc.walkingDirection,
441432
complexityBudget: getComplexityBudget(for: inst.parentFunction), context)
442433
{
443-
var effects = inst.memoryEffects
444-
if memLoc.isLetValue {
445-
effects.write = false
446-
}
447-
return effects
434+
return inst.memoryEffects
448435
}
449436
return .noEffects
450437
}

‎SwiftCompilerSources/Sources/Optimizer/TestPasses/MemBehaviorDumper.swift‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ private extension Instruction {
6565
is EndCOWMutationInst,
6666
is CopyValueInst,
6767
is DestroyValueInst,
68+
is StrongReleaseInst,
6869
is IsUniqueInst,
6970
is EndBorrowInst,
7071
is LoadInst,

‎test/SILOptimizer/copy-to-borrow-optimization.sil‎

Lines changed: 12 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ sil [ossa] @dont_copy_let_properties_with_guaranteed_base : $@convention(thin) (
199199
bb0(%x : @guaranteed $ClassLet):
200200
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
201201

202-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLet
202+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLet
203203
%v = load [copy] %p : $*Klass
204204
%b = begin_borrow %v : $Klass
205205
apply %f(%b) : $@convention(thin) (@guaranteed Klass) -> ()
@@ -223,7 +223,7 @@ sil [ossa] @dont_copy_let_properties_with_guaranteed_base_and_forwarding_uses :
223223
bb0(%x : @guaranteed $ClassLet):
224224
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
225225

226-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLet
226+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLet
227227
%v = load [copy] %p : $*Klass
228228
%c = unchecked_ref_cast %v : $Klass to $Klass
229229
%b = begin_borrow %c : $Klass
@@ -247,30 +247,7 @@ bb0(%x : @guaranteed $SubclassLet):
247247
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
248248

249249
%u = upcast %x : $SubclassLet to $ClassLet
250-
%p = ref_element_addr %u : $ClassLet, #ClassLet.aLet
251-
%v = load [copy] %p : $*Klass
252-
%b = begin_borrow %v : $Klass
253-
apply %f(%b) : $@convention(thin) (@guaranteed Klass) -> ()
254-
end_borrow %b : $Klass
255-
destroy_value %v : $Klass
256-
257-
return undef : $()
258-
}
259-
260-
// CHECK-LABEL: sil [ossa] @dont_copy_let_global :
261-
// CHECK: global_addr
262-
// CHECK-NEXT: load_borrow
263-
// CHECK-NEXT: begin_borrow
264-
// CHECK-NEXT: apply
265-
// CHECK-NEXT: end_borrow
266-
// CHECK-NEXT: end_borrow
267-
// CHECK-NEXT: return
268-
// CHECK-NEXT: } // end sil function 'dont_copy_let_global'
269-
sil [ossa] @dont_copy_let_global : $@convention(thin) () -> () {
270-
bb0:
271-
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
272-
273-
%p = global_addr @a_let_global : $*Klass
250+
%p = ref_element_addr [immutable] %u : $ClassLet, #ClassLet.aLet
274251
%v = load [copy] %p : $*Klass
275252
%b = begin_borrow %v : $Klass
276253
apply %f(%b) : $@convention(thin) (@guaranteed Klass) -> ()
@@ -293,7 +270,7 @@ sil [ossa] @dont_copy_let_properties_with_guaranteed_base_structural : $@convent
293270
bb0(%x : @guaranteed $ClassLet):
294271
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
295272

296-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLetTuple
273+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLetTuple
297274
%q = tuple_element_addr %p : $*(Klass, Klass), 1
298275
%v = load [copy] %q : $*Klass
299276
%b = begin_borrow %v : $Klass
@@ -369,7 +346,7 @@ bb0(%x : @owned $ClassLet):
369346
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
370347

371348
%a = begin_borrow %x : $ClassLet
372-
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
349+
%p = ref_element_addr [immutable] %a : $ClassLet, #ClassLet.aLet
373350
%v = load [copy] %p : $*Klass
374351
apply %f(%v) : $@convention(thin) (@guaranteed Klass) -> ()
375352
destroy_value %v : $Klass
@@ -409,7 +386,7 @@ bb0(%x : @owned $ClassLet):
409386
%f = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
410387

411388
%a = begin_borrow %x : $ClassLet
412-
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
389+
%p = ref_element_addr [immutable] %a : $ClassLet, #ClassLet.aLet
413390
%v = load [copy] %p : $*Klass
414391
%v_cast = unchecked_ref_cast %v : $Klass to $Builtin.NativeObject
415392
apply %f(%v_cast) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
@@ -1145,7 +1122,7 @@ bb0(%0 : @guaranteed $FakeOptional<ClassLet>):
11451122
switch_enum %0 : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
11461123

11471124
bb1(%1 : @guaranteed $ClassLet):
1148-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1125+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
11491126
%3 = load [copy] %2 : $*Klass
11501127
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
11511128
destroy_value %3 : $Klass
@@ -1171,7 +1148,7 @@ bb0(%0 : @owned $FakeOptional<ClassLet>):
11711148
switch_enum %0a : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
11721149

11731150
bb1(%1 : @guaranteed $ClassLet):
1174-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1151+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
11751152
%3 = load [copy] %2 : $*Klass
11761153
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
11771154
destroy_value %3 : $Klass
@@ -1201,7 +1178,7 @@ bb0(%0 : $*FakeOptional<ClassLet>):
12011178
switch_enum %0a : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
12021179

12031180
bb1(%1 : @guaranteed $ClassLet):
1204-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1181+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
12051182
%3 = load [copy] %2 : $*Klass
12061183
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12071184
destroy_value %3 : $Klass
@@ -1240,7 +1217,7 @@ bb0c(%0c : @guaranteed $FakeOptional<ClassLet>):
12401217
switch_enum %0d : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
12411218

12421219
bb1(%1 : @guaranteed $ClassLet):
1243-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1220+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
12441221
%3 = load [copy] %2 : $*Klass
12451222
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12461223
destroy_value %3 : $Klass
@@ -1276,7 +1253,7 @@ bb1:
12761253
bb2:
12771254
%0b = begin_borrow %0 : $FakeOptional<ClassLet>
12781255
%0b2 = unchecked_enum_data %0b : $FakeOptional<ClassLet>, #FakeOptional.some!enumelt
1279-
%2 = ref_element_addr %0b2 : $ClassLet, #ClassLet.aLet
1256+
%2 = ref_element_addr [immutable] %0b2 : $ClassLet, #ClassLet.aLet
12801257
%3 = load [copy] %2 : $*Klass
12811258
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12821259
destroy_value %3 : $Klass
@@ -1520,12 +1497,9 @@ bbEnd:
15201497
return %9999 : $()
15211498
}
15221499

1523-
// Just make sure that we do not crash on this code and convert the 2nd load
1524-
// [copy] to a load_borrow.
1500+
// Just make sure that we do not crash on this code
15251501
//
15261502
// CHECK-LABEL: sil [ossa] @improper_dead_end_block_crasher_test : $@convention(thin) (Builtin.RawPointer) -> () {
1527-
// CHECK: load_borrow
1528-
// CHECK: load_borrow
15291503
// CHECK: } // end sil function 'improper_dead_end_block_crasher_test'
15301504
sil [ossa] @improper_dead_end_block_crasher_test : $@convention(thin) (Builtin.RawPointer) -> () {
15311505
bb0(%0 : $Builtin.RawPointer):

‎test/SILOptimizer/mem-behavior.sil‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@ class C {
1919
@_hasStorage @_hasInitialValue final var prop: Builtin.Int32 { get }
2020
}
2121

22+
class CL {
23+
@_hasStorage let x: String
24+
}
25+
26+
2227
class Parent {
2328
@_hasStorage var child: C { get set }
2429
}
@@ -34,6 +39,7 @@ sil @nouser_func : $@convention(thin) () -> ()
3439
sil @in_ptr : $@convention(thin) (@in Builtin.RawPointer) -> ()
3540
sil @init_C : $@convention(thin) () -> @out C
3641
sil @read_C : $@convention(thin) (@in_guaranteed C) -> ()
42+
sil @consume : $@convention(thin) (@owned CL) -> ()
3743

3844
sil @store_to_int : $@convention(thin) (Int32, @inout Int32) -> () {
3945
[%1: write v**]
@@ -835,23 +841,6 @@ bb0(%0 : $*C, %1 : $*C):
835841
sil_global hidden [let] @globalC : $C
836842
sil_global hidden @globalCVar : $C
837843

838-
// CHECK-LABEL: @testGlobalLet
839-
// CHECK: PAIR #1.
840-
// CHECK-NEXT: %3 = apply %2() : $@convention(thin) () -> ()
841-
// CHECK-NEXT: %0 = global_addr @globalC : $*C
842-
// CHECK-NEXT: r=1,w=0
843-
sil hidden @testGlobalLet : $@convention(thin) () -> () {
844-
bb0:
845-
%0 = global_addr @globalC : $*C
846-
%1 = load %0 : $*C
847-
%2 = function_ref @nouser_func : $@convention(thin) () -> ()
848-
%3 = apply %2() : $@convention(thin) () -> ()
849-
%4 = function_ref @read_C : $@convention(thin) (@in_guaranteed C) -> ()
850-
%5 = apply %4(%0) : $@convention(thin) (@in_guaranteed C) -> ()
851-
%8 = tuple ()
852-
return %8 : $()
853-
}
854-
855844
// CHECK-LABEL: @testInitGlobalLet
856845
// CHECK: PAIR #0.
857846
// CHECK-NEXT: %2 = apply %1(%0) : $@convention(thin) () -> @out C
@@ -896,25 +885,6 @@ bb0:
896885
return %1 : $Builtin.RawPointer
897886
}
898887

899-
// CHECK-LABEL: @testGlobalViaAddressorLet
900-
// CHECK: PAIR #2.
901-
// CHECK-NEXT: %5 = apply %4() : $@convention(thin) () -> ()
902-
// CHECK-NEXT: %2 = pointer_to_address %1 : $Builtin.RawPointer to [strict] $*C
903-
// CHECK-NEXT: r=1,w=0
904-
sil @testGlobalViaAddressorLet : $@convention(thin) () -> () {
905-
bb0:
906-
%0 = function_ref @addressor_of_globalC : $@convention(thin) () -> Builtin.RawPointer
907-
%1 = apply %0() : $@convention(thin) () -> Builtin.RawPointer
908-
%2 = pointer_to_address %1 : $Builtin.RawPointer to [strict] $*C
909-
%3 = load %2 : $*C
910-
%4 = function_ref @nouser_func : $@convention(thin) () -> ()
911-
%5 = apply %4() : $@convention(thin) () -> ()
912-
%6 = function_ref @read_C : $@convention(thin) (@in_guaranteed C) -> ()
913-
%7 = apply %6(%2) : $@convention(thin) (@in_guaranteed C) -> ()
914-
%8 = tuple ()
915-
return %8 : $()
916-
}
917-
918888
// CHECK-LABEL: @testGlobalViaAddressorVar
919889
// CHECK: PAIR #2.
920890
// CHECK-NEXT: %5 = apply %4() : $@convention(thin) () -> ()
@@ -1788,3 +1758,45 @@ bb0(%0 : $*X):
17881758
return %2 : $C
17891759
}
17901760

1761+
// CHECK-LABEL: @test_release_of_class_with_let
1762+
// CHECK: PAIR #0.
1763+
// CHECK-NEXT: strong_release %0 : $CL
1764+
// CHECK-NEXT: %1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1765+
// CHECK-NEXT: r=1,w=1
1766+
sil @test_release_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1767+
bb0(%0 : $CL):
1768+
%1 = ref_element_addr [immutable] %0, #CL.x
1769+
strong_release %0
1770+
%3 = tuple ()
1771+
return %3
1772+
}
1773+
1774+
// CHECK-LABEL: @test_consume_of_class_with_let
1775+
// CHECK: PAIR #0.
1776+
// CHECK-NEXT: %3 = apply %2(%0) : $@convention(thin) (@owned CL) -> ()
1777+
// CHECK-NEXT: %1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1778+
// CHECK-NEXT: r=1,w=1
1779+
sil @test_consume_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1780+
bb0(%0 : $CL):
1781+
%1 = ref_element_addr [immutable] %0, #CL.x
1782+
%4 = function_ref @consume : $@convention(thin) (@owned CL) -> ()
1783+
apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1784+
%3 = tuple ()
1785+
return %3
1786+
}
1787+
1788+
// CHECK-LABEL: @test_ossa_consume_of_class_with_let
1789+
// CHECK: PAIR #1.
1790+
// CHECK-NEXT: %5 = apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1791+
// CHECK-NEXT: %2 = ref_element_addr [immutable] %1 : $CL, #CL.x
1792+
// CHECK-NEXT: r=1,w=1
1793+
sil [ossa] @test_ossa_consume_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1794+
bb0(%0 : @owned $CL):
1795+
%1 = begin_borrow %0
1796+
%2 = ref_element_addr [immutable] %1, #CL.x
1797+
end_borrow %1
1798+
%4 = function_ref @consume : $@convention(thin) (@owned CL) -> ()
1799+
apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1800+
%3 = tuple ()
1801+
return %3
1802+
}

‎test/SILOptimizer/temp_rvalue_opt.sil‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,15 @@ struct Str {
2525
var _value: Builtin.Int64
2626
}
2727

28+
class C {
29+
@_hasStorage let x: String
30+
}
31+
32+
2833
sil @unknown : $@convention(thin) () -> ()
2934
sil @load_string : $@convention(thin) (@in_guaranteed String) -> String
3035
sil @guaranteed_user : $@convention(thin) (@guaranteed Klass) -> ()
36+
sil @consume : $@convention(thin) (@owned C) -> ()
3137

3238
sil @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
3339
bb0(%0 : $*Klass):
@@ -761,3 +767,38 @@ bb0(%0 : $Klass, %1 : $Klass):
761767
dealloc_stack %3 : $*String
762768
return %323 : $String
763769
}
770+
771+
// CHECK-LABEL: sil @dont_remove_copy_from_released_object
772+
// CHECK: copy_addr
773+
// CHECK-NEXT: strong_release %0
774+
// CHECK: } // end sil function 'dont_remove_copy_from_released_object'
775+
sil @dont_remove_copy_from_released_object : $@convention(thin) (@owned C) -> @owned String {
776+
bb0(%0 : $C):
777+
%1 = ref_element_addr [immutable] %0, #C.x
778+
%2 = alloc_stack $String
779+
copy_addr %1 to [init] %2
780+
strong_release %0
781+
%5 = load %2
782+
retain_value %5
783+
destroy_addr %2
784+
dealloc_stack %2
785+
return %5
786+
}
787+
788+
// CHECK-LABEL: sil @dont_remove_copy_from_consumed_object
789+
// CHECK: copy_addr
790+
// CHECK: apply
791+
// CHECK: } // end sil function 'dont_remove_copy_from_consumed_object'
792+
sil @dont_remove_copy_from_consumed_object : $@convention(thin) (@owned C) -> @owned String {
793+
bb0(%0 : $C):
794+
%1 = ref_element_addr [immutable] %0, #C.x
795+
%2 = alloc_stack $String
796+
copy_addr %1 to [init] %2
797+
%4 = function_ref @consume : $@convention(thin) (@owned C) -> ()
798+
apply %4(%0) : $@convention(thin) (@owned C) -> ()
799+
%5 = load %2
800+
retain_value %5
801+
destroy_addr %2
802+
dealloc_stack %2
803+
return %5
804+
}

0 commit comments

Comments
 (0)