Skip to content

Commit ed26847

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 dc74591 commit ed26847

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/load-copy-to-borrow.sil‎

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

204-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLet
204+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLet
205205
%v = load [copy] %p : $*Klass
206206
%b = begin_borrow %v : $Klass
207207
apply %f(%b) : $@convention(thin) (@guaranteed Klass) -> ()
@@ -225,7 +225,7 @@ sil [ossa] @dont_copy_let_properties_with_guaranteed_base_and_forwarding_uses :
225225
bb0(%x : @guaranteed $ClassLet):
226226
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
227227

228-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLet
228+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLet
229229
%v = load [copy] %p : $*Klass
230230
%c = unchecked_ref_cast %v : $Klass to $Klass
231231
%b = begin_borrow %c : $Klass
@@ -249,30 +249,7 @@ bb0(%x : @guaranteed $SubclassLet):
249249
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
250250

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

298-
%p = ref_element_addr %x : $ClassLet, #ClassLet.aLetTuple
275+
%p = ref_element_addr [immutable] %x : $ClassLet, #ClassLet.aLetTuple
299276
%q = tuple_element_addr %p : $*(Klass, Klass), 1
300277
%v = load [copy] %q : $*Klass
301278
%b = begin_borrow %v : $Klass
@@ -371,7 +348,7 @@ bb0(%x : @owned $ClassLet):
371348
%f = function_ref @black_hole : $@convention(thin) (@guaranteed Klass) -> ()
372349

373350
%a = begin_borrow %x : $ClassLet
374-
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
351+
%p = ref_element_addr [immutable] %a : $ClassLet, #ClassLet.aLet
375352
%v = load [copy] %p : $*Klass
376353
apply %f(%v) : $@convention(thin) (@guaranteed Klass) -> ()
377354
destroy_value %v : $Klass
@@ -411,7 +388,7 @@ bb0(%x : @owned $ClassLet):
411388
%f = function_ref @guaranteed_user : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
412389

413390
%a = begin_borrow %x : $ClassLet
414-
%p = ref_element_addr %a : $ClassLet, #ClassLet.aLet
391+
%p = ref_element_addr [immutable] %a : $ClassLet, #ClassLet.aLet
415392
%v = load [copy] %p : $*Klass
416393
%v_cast = unchecked_ref_cast %v : $Klass to $Builtin.NativeObject
417394
apply %f(%v_cast) : $@convention(thin) (@guaranteed Builtin.NativeObject) -> ()
@@ -1147,7 +1124,7 @@ bb0(%0 : @guaranteed $FakeOptional<ClassLet>):
11471124
switch_enum %0 : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
11481125

11491126
bb1(%1 : @guaranteed $ClassLet):
1150-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1127+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
11511128
%3 = load [copy] %2 : $*Klass
11521129
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
11531130
destroy_value %3 : $Klass
@@ -1173,7 +1150,7 @@ bb0(%0 : @owned $FakeOptional<ClassLet>):
11731150
switch_enum %0a : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
11741151

11751152
bb1(%1 : @guaranteed $ClassLet):
1176-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1153+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
11771154
%3 = load [copy] %2 : $*Klass
11781155
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
11791156
destroy_value %3 : $Klass
@@ -1203,7 +1180,7 @@ bb0(%0 : $*FakeOptional<ClassLet>):
12031180
switch_enum %0a : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
12041181

12051182
bb1(%1 : @guaranteed $ClassLet):
1206-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1183+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
12071184
%3 = load [copy] %2 : $*Klass
12081185
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12091186
destroy_value %3 : $Klass
@@ -1242,7 +1219,7 @@ bb0c(%0c : @guaranteed $FakeOptional<ClassLet>):
12421219
switch_enum %0d : $FakeOptional<ClassLet>, case #FakeOptional.some!enumelt: bb1, case #FakeOptional.none!enumelt: bb2
12431220

12441221
bb1(%1 : @guaranteed $ClassLet):
1245-
%2 = ref_element_addr %1 : $ClassLet, #ClassLet.aLet
1222+
%2 = ref_element_addr [immutable] %1 : $ClassLet, #ClassLet.aLet
12461223
%3 = load [copy] %2 : $*Klass
12471224
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12481225
destroy_value %3 : $Klass
@@ -1278,7 +1255,7 @@ bb1:
12781255
bb2:
12791256
%0b = begin_borrow %0 : $FakeOptional<ClassLet>
12801257
%0b2 = unchecked_enum_data %0b : $FakeOptional<ClassLet>, #FakeOptional.some!enumelt
1281-
%2 = ref_element_addr %0b2 : $ClassLet, #ClassLet.aLet
1258+
%2 = ref_element_addr [immutable] %0b2 : $ClassLet, #ClassLet.aLet
12821259
%3 = load [copy] %2 : $*Klass
12831260
apply %f(%3) : $@convention(thin) (@guaranteed Klass) -> ()
12841261
destroy_value %3 : $Klass
@@ -1522,12 +1499,9 @@ bbEnd:
15221499
return %9999 : $()
15231500
}
15241501

1525-
// Just make sure that we do not crash on this code and convert the 2nd load
1526-
// [copy] to a load_borrow.
1502+
// Just make sure that we do not crash on this code
15271503
//
15281504
// CHECK-LABEL: sil [ossa] @improper_dead_end_block_crasher_test : $@convention(thin) (Builtin.RawPointer) -> () {
1529-
// CHECK: load_borrow
1530-
// CHECK: load_borrow
15311505
// CHECK: } // end sil function 'improper_dead_end_block_crasher_test'
15321506
sil [ossa] @improper_dead_end_block_crasher_test : $@convention(thin) (Builtin.RawPointer) -> () {
15331507
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) () -> ()
@@ -1769,3 +1739,45 @@ bb0(%0 : $*X):
17691739
return %2 : $C
17701740
}
17711741

1742+
// CHECK-LABEL: @test_release_of_class_with_let
1743+
// CHECK: PAIR #0.
1744+
// CHECK-NEXT: strong_release %0 : $CL
1745+
// CHECK-NEXT: %1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1746+
// CHECK-NEXT: r=1,w=1
1747+
sil @test_release_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1748+
bb0(%0 : $CL):
1749+
%1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1750+
strong_release %0 : $CL
1751+
%3 = tuple ()
1752+
return %3 : $()
1753+
}
1754+
1755+
// CHECK-LABEL: @test_consume_of_class_with_let
1756+
// CHECK: PAIR #0.
1757+
// CHECK-NEXT: %3 = apply %2(%0) : $@convention(thin) (@owned CL) -> ()
1758+
// CHECK-NEXT: %1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1759+
// CHECK-NEXT: r=1,w=1
1760+
sil @test_consume_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1761+
bb0(%0 : $CL):
1762+
%1 = ref_element_addr [immutable] %0 : $CL, #CL.x
1763+
%4 = function_ref @consume : $@convention(thin) (@owned CL) -> ()
1764+
apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1765+
%3 = tuple ()
1766+
return %3 : $()
1767+
}
1768+
1769+
// CHECK-LABEL: @test_ossa_consume_of_class_with_let
1770+
// CHECK: PAIR #1.
1771+
// CHECK-NEXT: %5 = apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1772+
// CHECK-NEXT: %2 = ref_element_addr [immutable] %1 : $CL, #CL.x
1773+
// CHECK-NEXT: r=1,w=1
1774+
sil [ossa] @test_ossa_consume_of_class_with_let : $@convention(thin) (@owned CL) -> () {
1775+
bb0(%0 : @owned $CL):
1776+
%1 = begin_borrow %0 : $CL
1777+
%2 = ref_element_addr [immutable] %1 : $CL, #CL.x
1778+
end_borrow %1 : $CL
1779+
%4 = function_ref @consume : $@convention(thin) (@owned CL) -> ()
1780+
apply %4(%0) : $@convention(thin) (@owned CL) -> ()
1781+
%3 = tuple ()
1782+
return %3 : $()
1783+
}

‎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, #C.x
778+
%2 = alloc_stack $String
779+
copy_addr %1 to [init] %2 : $*String
780+
strong_release %0 : $C
781+
%5 = load %2 : $*String
782+
retain_value %5 : $String
783+
destroy_addr %2 : $*String
784+
dealloc_stack %2 : $*String
785+
return %5 : $String
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, #C.x
795+
%2 = alloc_stack $String
796+
copy_addr %1 to [init] %2 : $*String
797+
%4 = function_ref @consume : $@convention(thin) (@owned C) -> ()
798+
apply %4(%0) : $@convention(thin) (@owned C) -> ()
799+
%5 = load %2 : $*String
800+
retain_value %5 : $String
801+
destroy_addr %2 : $*String
802+
dealloc_stack %2 : $*String
803+
return %5 : $String
804+
}

0 commit comments

Comments
 (0)