Skip to content

Commit 12fcb35

Browse files
kavonkavon
committed
fix for SR-15789
The problem is in `OptimizeHopToExecutor::collectActors`, which is causing `removeRedundantHopToExecutors` to drop the `hop_to_executor`. The issue is triggered by the order in which the Actors map is populated, and the reuse of a SILValue representing an executor. Suppose we have a function like this: ``` bb0: %7 = builtin "getCurrentExecutor"() : $Optional<Builtin.Executor> // ... cond_br %12, bb2, bb1 bb1: // ... hop_to_executor %7 : $Optional<Builtin.Executor> bb2: // ... hop_to_executor %7 : $Optional<Builtin.Executor> // ... hop_to_executor %28 : $MainActor ``` Since `collectActors` goes through each function's instructions top-down, and assigns to each unique SILValue an ID equal to the size of the Actors map prior to inserting the SILValue in the map, and we end up with the wrong actor IDs. This commit changes the ID numbering scheme to correct the issue.
1 parent 9839378 commit 12fcb35

File tree

2 files changed

+288
-2
lines changed

2 files changed

+288
-2
lines changed

‎lib/SILOptimizer/Mandatory/OptimizeHopToExecutor.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,16 @@ class OptimizeHopToExecutor {
118118

119119
/// Search for hop_to_executor instructions and add their operands to \p actors.
120120
void OptimizeHopToExecutor::collectActors(Actors &actors) {
121+
int uniqueActorID = 0;
121122
for (SILBasicBlock &block : *function) {
122123
for (SILInstruction &inst : block) {
123124
if (auto *hop = dyn_cast<HopToExecutorInst>(&inst)) {
124-
int idx = actors.size();
125-
actors[hop->getOperand()] = idx;
125+
auto oper = hop->getOperand();
126+
127+
if (actors.count(oper))
128+
continue;
129+
130+
actors[oper] = uniqueActorID++;
126131
}
127132
}
128133
}

‎test/SILOptimizer/optimize_hop_to_executor2.sil

Lines changed: 281 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,281 @@
1+
// RUN: %target-swift-frontend %s -emit-sil | %FileCheck %s
2+
3+
// REQUIRES: concurrency
4+
// REQUIRES: objc_interop
5+
6+
// CHECK-LABEL: sil private @$s3bug7trigger1pyAA9PresenterC_tYaFytSgyYaXEfU_ : $@convention(thin) @async (@guaranteed Presenter) -> Optional<()>
7+
// CHECK: bb2:
8+
// CHECK: hop_to_executor {{%.+}} : $MainActor
9+
10+
sil_stage raw
11+
12+
import Builtin
13+
import Swift
14+
import SwiftShims
15+
import _Concurrency
16+
17+
func trigger(p: Presenter) async
18+
19+
final class Presenter {
20+
@MainActor @_hasStorage @_hasInitialValue final var view: Vista? { get set }
21+
@objc deinit
22+
init()
23+
}
24+
25+
class Vista {
26+
@MainActor func doSmthOnMain()
27+
@objc deinit
28+
init()
29+
}
30+
31+
@MainActor @_hasStorage @_hasInitialValue var x: Int { get set }
32+
33+
// x
34+
sil_global hidden @$s3bug1xSivp : $Int
35+
36+
// main
37+
sil [ossa] @main : $@convention(c) (Int32, UnsafeMutablePointer<Optional<UnsafeMutablePointer<Int8>>>) -> Int32 {
38+
bb0(%0 : $Int32, %1 : $UnsafeMutablePointer<Optional<UnsafeMutablePointer<Int8>>>):
39+
alloc_global @$s3bug1xSivp // id: %2
40+
%3 = global_addr @$s3bug1xSivp : $*Int // user: %8
41+
%4 = integer_literal $Builtin.IntLiteral, 0 // user: %7
42+
%5 = metatype $@thin Int.Type // user: %7
43+
// function_ref Int.init(_builtinIntegerLiteral:)
44+
%6 = function_ref @$sSi22_builtinIntegerLiteralSiBI_tcfC : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %7
45+
%7 = apply %6(%4, %5) : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %8
46+
store %7 to [trivial] %3 : $*Int // id: %8
47+
%9 = integer_literal $Builtin.Int32, 0 // user: %10
48+
%10 = struct $Int32 (%9 : $Builtin.Int32) // user: %11
49+
return %10 : $Int32 // id: %11
50+
} // end sil function 'main'
51+
52+
// trigger(p:)
53+
sil hidden [ossa] @$s3bug7trigger1pyAA9PresenterC_tYaF : $@convention(thin) @async (@guaranteed Presenter) -> () {
54+
// %0 "p" // users: %3, %1
55+
bb0(%0 : @guaranteed $Presenter):
56+
debug_value %0 : $Presenter, let, name "p", argno 1 // id: %1
57+
// function_ref closure #1 in trigger(p:)
58+
%2 = function_ref @$s3bug7trigger1pyAA9PresenterC_tYaFytSgyYaXEfU_ : $@convention(thin) @async (@guaranteed Presenter) -> Optional<()> // user: %3
59+
%3 = apply %2(%0) : $@convention(thin) @async (@guaranteed Presenter) -> Optional<()>
60+
%4 = tuple () // user: %5
61+
return %4 : $() // id: %5
62+
} // end sil function '$s3bug7trigger1pyAA9PresenterC_tYaF'
63+
64+
// closure #1 in trigger(p:)
65+
sil private [ossa] @$s3bug7trigger1pyAA9PresenterC_tYaFytSgyYaXEfU_ : $@convention(thin) @async (@guaranteed Presenter) -> Optional<()> {
66+
// %0 "p" // users: %2, %1
67+
bb0(%0 : @guaranteed $Presenter):
68+
debug_value %0 : $Presenter, let, name "p", argno 1 // id: %1
69+
%2 = ref_element_addr %0 : $Presenter, #Presenter.view // user: %9
70+
%3 = metatype $@thick MainActor.Type // user: %5
71+
// function_ref static MainActor.shared.getter
72+
%4 = function_ref @$sScM6sharedScMvgZ : $@convention(method) (@thick MainActor.Type) -> @owned MainActor // user: %5
73+
%5 = apply %4(%3) : $@convention(method) (@thick MainActor.Type) -> @owned MainActor // users: %36, %17, %6
74+
%6 = begin_borrow %5 : $MainActor // users: %23, %16, %8
75+
%7 = builtin "getCurrentExecutor"() : $Optional<Builtin.Executor> // users: %22, %15
76+
hop_to_executor %6 : $MainActor // id: %8
77+
%9 = begin_access [read] [dynamic] %2 : $*Optional<Vista> // users: %21, %19, %14, %12
78+
%10 = integer_literal $Builtin.Int1, -1 // user: %12
79+
%11 = integer_literal $Builtin.Int1, 0 // user: %12
80+
%12 = select_enum_addr %9 : $*Optional<Vista>, case #Optional.some!enumelt: %10, default %11 : $Builtin.Int1 // user: %13
81+
cond_br %12, bb2, bb1 // id: %13
82+
83+
bb1: // Preds: bb0
84+
end_access %9 : $*Optional<Vista> // id: %14
85+
hop_to_executor %7 : $Optional<Builtin.Executor> // id: %15
86+
end_borrow %6 : $MainActor // id: %16
87+
destroy_value %5 : $MainActor // id: %17
88+
br bb4 // id: %18
89+
90+
bb2: // Preds: bb0
91+
%19 = unchecked_take_enum_data_addr %9 : $*Optional<Vista>, #Optional.some!enumelt // user: %20
92+
%20 = load [copy] %19 : $*Vista // users: %35, %31, %24
93+
end_access %9 : $*Optional<Vista> // id: %21
94+
hop_to_executor %7 : $Optional<Builtin.Executor> // id: %22
95+
end_borrow %6 : $MainActor // id: %23
96+
%24 = class_method %20 : $Vista, #Vista.doSmthOnMain : (Vista) -> () -> (), $@convention(method) (@guaranteed Vista) -> () // user: %31
97+
%25 = metatype $@thick MainActor.Type // user: %27
98+
// function_ref static MainActor.shared.getter
99+
%26 = function_ref @$sScM6sharedScMvgZ : $@convention(method) (@thick MainActor.Type) -> @owned MainActor // user: %27
100+
%27 = apply %26(%25) : $@convention(method) (@thick MainActor.Type) -> @owned MainActor // users: %34, %28
101+
%28 = begin_borrow %27 : $MainActor // users: %33, %30
102+
%29 = builtin "getCurrentExecutor"() : $Optional<Builtin.Executor> // user: %32
103+
hop_to_executor %28 : $MainActor // id: %30
104+
%31 = apply %24(%20) : $@convention(method) (@guaranteed Vista) -> ()
105+
hop_to_executor %29 : $Optional<Builtin.Executor> // id: %32
106+
end_borrow %28 : $MainActor // id: %33
107+
destroy_value %27 : $MainActor // id: %34
108+
destroy_value %20 : $Vista // id: %35
109+
destroy_value %5 : $MainActor // id: %36
110+
%37 = tuple () // user: %38
111+
%38 = enum $Optional<()>, #Optional.some!enumelt, %37 : $() // user: %39
112+
br bb3(%38 : $Optional<()>) // id: %39
113+
114+
// %40 // user: %41
115+
bb3(%40 : $Optional<()>): // Preds: bb4 bb2
116+
return %40 : $Optional<()> // id: %41
117+
118+
bb4: // Preds: bb1
119+
%42 = enum $Optional<()>, #Optional.none!enumelt // user: %43
120+
br bb3(%42 : $Optional<()>) // id: %43
121+
} // end sil function '$s3bug7trigger1pyAA9PresenterC_tYaFytSgyYaXEfU_'
122+
123+
// static MainActor.shared.getter
124+
sil [available 12.0.0] @$sScM6sharedScMvgZ : $@convention(method) (@thick MainActor.Type) -> @owned MainActor
125+
126+
// variable initialization expression of Presenter.view
127+
sil hidden [transparent] [ossa] @$s3bug9PresenterC4viewAA5VistaCSgvpfi : $@convention(thin) () -> @owned Optional<Vista> {
128+
bb0:
129+
%0 = metatype $@thick Vista.Type // user: %2
130+
// function_ref Vista.__allocating_init()
131+
%1 = function_ref @$s3bug5VistaCACycfC : $@convention(method) (@thick Vista.Type) -> @owned Vista // user: %2
132+
%2 = apply %1(%0) : $@convention(method) (@thick Vista.Type) -> @owned Vista // user: %3
133+
%3 = enum $Optional<Vista>, #Optional.some!enumelt, %2 : $Vista // user: %4
134+
return %3 : $Optional<Vista> // id: %4
135+
} // end sil function '$s3bug9PresenterC4viewAA5VistaCSgvpfi'
136+
137+
// Vista.__allocating_init()
138+
sil hidden [exact_self_class] [ossa] @$s3bug5VistaCACycfC : $@convention(method) (@thick Vista.Type) -> @owned Vista {
139+
// %0 "$metatype"
140+
bb0(%0 : $@thick Vista.Type):
141+
%1 = alloc_ref $Vista // user: %3
142+
// function_ref Vista.init()
143+
%2 = function_ref @$s3bug5VistaCACycfc : $@convention(method) (@owned Vista) -> @owned Vista // user: %3
144+
%3 = apply %2(%1) : $@convention(method) (@owned Vista) -> @owned Vista // user: %4
145+
return %3 : $Vista // id: %4
146+
} // end sil function '$s3bug5VistaCACycfC'
147+
148+
// Presenter.deinit
149+
sil hidden [ossa] @$s3bug9PresenterCfd : $@convention(method) (@guaranteed Presenter) -> @owned Builtin.NativeObject {
150+
// %0 "self" // users: %6, %2, %1
151+
bb0(%0 : @guaranteed $Presenter):
152+
debug_value %0 : $Presenter, let, name "self", argno 1, implicit // id: %1
153+
%2 = ref_element_addr %0 : $Presenter, #Presenter.view // user: %3
154+
%3 = begin_access [deinit] [static] %2 : $*Optional<Vista> // users: %5, %4
155+
destroy_addr %3 : $*Optional<Vista> // id: %4
156+
end_access %3 : $*Optional<Vista> // id: %5
157+
%6 = unchecked_ref_cast %0 : $Presenter to $Builtin.NativeObject // user: %7
158+
%7 = unchecked_ownership_conversion %6 : $Builtin.NativeObject, @guaranteed to @owned // user: %8
159+
return %7 : $Builtin.NativeObject // id: %8
160+
} // end sil function '$s3bug9PresenterCfd'
161+
162+
// Presenter.__deallocating_deinit
163+
sil hidden [ossa] @$s3bug9PresenterCfD : $@convention(method) (@owned Presenter) -> () {
164+
// %0 "self" // users: %6, %3, %1
165+
bb0(%0 : @owned $Presenter):
166+
debug_value %0 : $Presenter, let, name "self", argno 1, implicit // id: %1
167+
// function_ref Presenter.deinit
168+
%2 = function_ref @$s3bug9PresenterCfd : $@convention(method) (@guaranteed Presenter) -> @owned Builtin.NativeObject // user: %4
169+
%3 = begin_borrow %0 : $Presenter // users: %5, %4
170+
%4 = apply %2(%3) : $@convention(method) (@guaranteed Presenter) -> @owned Builtin.NativeObject // user: %7
171+
end_borrow %3 : $Presenter // id: %5
172+
end_lifetime %0 : $Presenter // id: %6
173+
%7 = unchecked_ref_cast %4 : $Builtin.NativeObject to $Presenter // user: %8
174+
dealloc_ref %7 : $Presenter // id: %8
175+
%9 = tuple () // user: %10
176+
return %9 : $() // id: %10
177+
} // end sil function '$s3bug9PresenterCfD'
178+
179+
// Presenter.__allocating_init()
180+
sil hidden [exact_self_class] [ossa] @$s3bug9PresenterCACycfC : $@convention(method) (@thick Presenter.Type) -> @owned Presenter {
181+
// %0 "$metatype"
182+
bb0(%0 : $@thick Presenter.Type):
183+
%1 = alloc_ref $Presenter // user: %3
184+
// function_ref Presenter.init()
185+
%2 = function_ref @$s3bug9PresenterCACycfc : $@convention(method) (@owned Presenter) -> @owned Presenter // user: %3
186+
%3 = apply %2(%1) : $@convention(method) (@owned Presenter) -> @owned Presenter // user: %4
187+
return %3 : $Presenter // id: %4
188+
} // end sil function '$s3bug9PresenterCACycfC'
189+
190+
// Presenter.init()
191+
sil hidden [ossa] @$s3bug9PresenterCACycfc : $@convention(method) (@owned Presenter) -> @owned Presenter {
192+
// %0 "self" // users: %2, %1
193+
bb0(%0 : @owned $Presenter):
194+
debug_value %0 : $Presenter, let, name "self", argno 1, implicit // id: %1
195+
%2 = mark_uninitialized [rootself] %0 : $Presenter // users: %10, %9, %3
196+
%3 = begin_borrow %2 : $Presenter // users: %8, %4
197+
%4 = ref_element_addr %3 : $Presenter, #Presenter.view // user: %7
198+
// function_ref variable initialization expression of Presenter.view
199+
%5 = function_ref @$s3bug9PresenterC4viewAA5VistaCSgvpfi : $@convention(thin) () -> @owned Optional<Vista> // user: %6
200+
%6 = apply %5() : $@convention(thin) () -> @owned Optional<Vista> // user: %7
201+
store %6 to [init] %4 : $*Optional<Vista> // id: %7
202+
end_borrow %3 : $Presenter // id: %8
203+
%9 = copy_value %2 : $Presenter // user: %11
204+
destroy_value %2 : $Presenter // id: %10
205+
return %9 : $Presenter // id: %11
206+
} // end sil function '$s3bug9PresenterCACycfc'
207+
208+
// Vista.doSmthOnMain()
209+
sil hidden [ossa] @$s3bug5VistaC12doSmthOnMainyyF : $@convention(method) (@guaranteed Vista) -> () {
210+
// %0 "self" // user: %2
211+
bb0(%0 : @guaranteed $Vista):
212+
%1 = global_addr @$s3bug1xSivp : $*Int // user: %7
213+
debug_value %0 : $Vista, let, name "self", argno 1, implicit // id: %2
214+
%3 = integer_literal $Builtin.IntLiteral, 1 // user: %6
215+
%4 = metatype $@thin Int.Type // user: %6
216+
// function_ref Int.init(_builtinIntegerLiteral:)
217+
%5 = function_ref @$sSi22_builtinIntegerLiteralSiBI_tcfC : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %6
218+
%6 = apply %5(%3, %4) : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %8
219+
%7 = begin_access [modify] [dynamic] %1 : $*Int // users: %9, %8
220+
assign %6 to %7 : $*Int // id: %8
221+
end_access %7 : $*Int // id: %9
222+
%10 = tuple () // user: %11
223+
return %10 : $() // id: %11
224+
} // end sil function '$s3bug5VistaC12doSmthOnMainyyF'
225+
226+
// Int.init(_builtinIntegerLiteral:)
227+
sil [transparent] [serialized] @$sSi22_builtinIntegerLiteralSiBI_tcfC : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int
228+
229+
// Vista.deinit
230+
sil hidden [ossa] @$s3bug5VistaCfd : $@convention(method) (@guaranteed Vista) -> @owned Builtin.NativeObject {
231+
// %0 "self" // users: %2, %1
232+
bb0(%0 : @guaranteed $Vista):
233+
debug_value %0 : $Vista, let, name "self", argno 1, implicit // id: %1
234+
%2 = unchecked_ref_cast %0 : $Vista to $Builtin.NativeObject // user: %3
235+
%3 = unchecked_ownership_conversion %2 : $Builtin.NativeObject, @guaranteed to @owned // user: %4
236+
return %3 : $Builtin.NativeObject // id: %4
237+
} // end sil function '$s3bug5VistaCfd'
238+
239+
// Vista.__deallocating_deinit
240+
sil hidden [ossa] @$s3bug5VistaCfD : $@convention(method) (@owned Vista) -> () {
241+
// %0 "self" // users: %6, %3, %1
242+
bb0(%0 : @owned $Vista):
243+
debug_value %0 : $Vista, let, name "self", argno 1, implicit // id: %1
244+
// function_ref Vista.deinit
245+
%2 = function_ref @$s3bug5VistaCfd : $@convention(method) (@guaranteed Vista) -> @owned Builtin.NativeObject // user: %4
246+
%3 = begin_borrow %0 : $Vista // users: %5, %4
247+
%4 = apply %2(%3) : $@convention(method) (@guaranteed Vista) -> @owned Builtin.NativeObject // user: %7
248+
end_borrow %3 : $Vista // id: %5
249+
end_lifetime %0 : $Vista // id: %6
250+
%7 = unchecked_ref_cast %4 : $Builtin.NativeObject to $Vista // user: %8
251+
dealloc_ref %7 : $Vista // id: %8
252+
%9 = tuple () // user: %10
253+
return %9 : $() // id: %10
254+
} // end sil function '$s3bug5VistaCfD'
255+
256+
// Vista.init()
257+
sil hidden [ossa] @$s3bug5VistaCACycfc : $@convention(method) (@owned Vista) -> @owned Vista {
258+
// %0 "self" // users: %2, %1
259+
bb0(%0 : @owned $Vista):
260+
debug_value %0 : $Vista, let, name "self", argno 1, implicit // id: %1
261+
%2 = mark_uninitialized [rootself] %0 : $Vista // users: %4, %3
262+
%3 = copy_value %2 : $Vista // user: %5
263+
destroy_value %2 : $Vista // id: %4
264+
return %3 : $Vista // id: %5
265+
} // end sil function '$s3bug5VistaCACycfc'
266+
267+
sil_vtable Presenter {
268+
#Presenter.init!allocator: (Presenter.Type) -> () -> Presenter : @$s3bug9PresenterCACycfC // Presenter.__allocating_init()
269+
#Presenter.deinit!deallocator: @$s3bug9PresenterCfD // Presenter.__deallocating_deinit
270+
}
271+
272+
sil_vtable Vista {
273+
#Vista.doSmthOnMain: (Vista) -> () -> () : @$s3bug5VistaC12doSmthOnMainyyF // Vista.doSmthOnMain()
274+
#Vista.init!allocator: (Vista.Type) -> () -> Vista : @$s3bug5VistaCACycfC // Vista.__allocating_init()
275+
#Vista.deinit!deallocator: @$s3bug5VistaCfD // Vista.__deallocating_deinit
276+
}
277+
278+
279+
280+
// Mappings from '#fileID' to '#filePath':
281+
// 'bug/explore.swift' => 'explore.swift'

0 commit comments

Comments
 (0)