Skip to content

Commit fef7f53

Browse files
gottesmmgottesmm
authored
Merge pull request #83295 from gottesmm/pr-0b35d691a2ceae00c6e9727dd57aeedb631d207c
[concurrency] Fix optimize_hop_to_executor so that we take advantage of the new nonisolated(nonsending) ABI in post 6.2.
2 parents e251aea + 734f057 commit fef7f53

File tree

4 files changed

+56
-61
lines changed

4 files changed

+56
-61
lines changed

‎lib/SILOptimizer/Mandatory/OptimizeHopToExecutor.cpp‎

Lines changed: 45 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,12 @@ class OptimizeHopToExecutor {
119119
/// Search for hop_to_executor instructions and add their operands to \p actors.
120120
void OptimizeHopToExecutor::collectActors(Actors &actors) {
121121
int uniqueActorID = 0;
122+
123+
if (auto isolation = function->getActorIsolation();
124+
isolation && isolation->isCallerIsolationInheriting()) {
125+
actors[function->maybeGetIsolatedArgument()] = uniqueActorID++;
126+
}
127+
122128
for (SILBasicBlock &block : *function) {
123129
for (SILInstruction &inst : block) {
124130
if (auto *hop = dyn_cast<HopToExecutorInst>(&inst)) {
@@ -193,10 +199,7 @@ void OptimizeHopToExecutor::solveDataflowBackward() {
193199
/// Returns true if \p inst is a suspension point or an async call.
194200
static bool isSuspensionPoint(SILInstruction *inst) {
195201
if (auto applySite = FullApplySite::isa(inst)) {
196-
// NOTE: For 6.2, we consider nonisolated(nonsending) to be a suspension
197-
// point, when it really isn't. We do this so that we have a truly
198-
// conservative change that does not change output.
199-
if (applySite.isAsync())
202+
if (applySite.isAsync() && !applySite.isCallerIsolationInheriting())
200203
return true;
201204
return false;
202205
}
@@ -213,8 +216,20 @@ bool OptimizeHopToExecutor::removeRedundantHopToExecutors(const Actors &actors)
213216

214217
// Initialize the dataflow.
215218
for (BlockState &state : blockStates) {
216-
state.entry = (state.block == function->getEntryBlock() ?
217-
BlockState::Unknown : BlockState::NotSet);
219+
state.entry = [&]() -> int {
220+
if (state.block != function->getEntryBlock()) {
221+
return BlockState::NotSet;
222+
}
223+
224+
if (auto isolation = function->getActorIsolation();
225+
isolation && isolation->isCallerIsolationInheriting()) {
226+
auto *fArg =
227+
cast<SILFunctionArgument>(function->maybeGetIsolatedArgument());
228+
return actors.lookup(SILValue(fArg));
229+
}
230+
231+
return BlockState::Unknown;
232+
}();
218233
state.intra = BlockState::NotSet;
219234
for (SILInstruction &inst : *state.block) {
220235
if (isSuspensionPoint(&inst)) {
@@ -316,44 +331,11 @@ void OptimizeHopToExecutor::updateNeedExecutor(int &needExecutor,
316331
return;
317332
}
318333

319-
// For 6.2 to be conservative, if we are calling a function with
320-
// caller_isolation_inheriting isolation, treat the callsite as if the
321-
// callsite is an instruction that needs an executor.
322-
//
323-
// DISCUSSION: The reason why we are doing this is that in 6.2, we are going
324-
// to continue treating caller isolation inheriting functions as a suspension
325-
// point for the purpose of eliminating redundant hop to executor to not make
326-
// this optimization more aggressive. Post 6.2, we will stop treating caller
327-
// isolation inheriting functions as suspension points, meaning this code can
328-
// be deleted.
329-
if (auto fas = FullApplySite::isa(inst);
330-
fas && fas.isAsync() && fas.isCallerIsolationInheriting()) {
331-
needExecutor = BlockState::ExecutorNeeded;
332-
return;
333-
}
334-
335-
// For 6.2, if we are in a caller isolation inheriting function, we need to
336-
// treat its return as an executor needing function before
337-
// isSuspensionPoint.
338-
//
339-
// DISCUSSION: We need to do this here since for 6.2, a caller isolation
340-
// inheriting function is going to be considered a suspension point to be
341-
// conservative and make this optimization strictly more conservative. Post
342-
// 6.2, since caller isolation inheriting functions will no longer be
343-
// considered suspension points, we will be able to sink this code into needs
344-
// executor.
345-
if (isa<ReturnInst>(inst)) {
346-
if (auto isolation = inst->getFunction()->getActorIsolation();
347-
isolation && isolation->isCallerIsolationInheriting()) {
348-
needExecutor = BlockState::ExecutorNeeded;
349-
return;
350-
}
351-
}
352-
353334
if (isSuspensionPoint(inst)) {
354335
needExecutor = BlockState::NoExecutorNeeded;
355336
return;
356337
}
338+
357339
if (needsExecutor(inst))
358340
needExecutor = BlockState::ExecutorNeeded;
359341
}
@@ -403,6 +385,29 @@ bool OptimizeHopToExecutor::needsExecutor(SILInstruction *inst) {
403385
if (isa<BeginBorrowInst>(inst) || isa<EndBorrowInst>(inst)) {
404386
return false;
405387
}
388+
389+
// A call to a caller isolation inheriting function does not create dead
390+
// executors since caller isolation inheriting functions do not hop in their
391+
// prologue.
392+
if (auto fas = FullApplySite::isa(inst);
393+
fas && fas.isAsync() && fas.isCallerIsolationInheriting()) {
394+
return true;
395+
}
396+
397+
// Treat returns from a caller isolation inheriting function as requiring the
398+
// liveness of hop to executors before it.
399+
//
400+
// DISCUSSION: We do this since callers of callee functions with isolation
401+
// inheriting isolation are not required to have a hop after the return from
402+
// the callee function... so we have no guarantee that there isn't code in the
403+
// caller that needs this hop to executor to run on the correct actor.
404+
if (isa<ReturnInst>(inst)) {
405+
if (auto isolation = inst->getFunction()->getActorIsolation();
406+
isolation && isolation->isCallerIsolationInheriting()) {
407+
return true;
408+
}
409+
}
410+
406411
return inst->mayReadOrWriteMemory();
407412
}
408413

‎test/Concurrency/isolated_nonsending_isolation_macro_sil.swift‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,10 @@ func take(iso: (any Actor)?) {}
1212
// CHECK-LABEL: // nonisolatedNonsending()
1313
// CHECK-NEXT: // Isolation: caller_isolation_inheriting
1414
// CHECK-NEXT: sil hidden @$s39isolated_nonsending_isolation_macro_sil21nonisolatedNonsendingyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
15-
// CHECK: bb0(%0 : $Optional<any Actor>):
16-
// CHECK-NEXT: hop_to_executor %0 // id: %1
17-
// CHECK-NEXT: retain_value %0 // id: %2
18-
// CHECK-NEXT: debug_value %0, let, name "iso" // id: %3
19-
// CHECK-NEXT: // function_ref take(iso:)
20-
// CHECK-NEXT: %4 = function_ref @$s39isolated_nonsending_isolation_macro_sil4take3isoyScA_pSg_tF : $@convention(thin) (@guaranteed Optional<any Actor>) -> () // user: %5
21-
// CHECK-NEXT: %5 = apply %4(%0) : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
22-
// CHECK-NEXT: release_value %0 // id: %6
23-
// CHECK-NEXT: %7 = tuple () // user: %8
24-
// CHECK-NEXT: return %7 // id: %8
25-
// CHECK-NEXT: } // end sil function '$s39isolated_nonsending_isolation_macro_sil21nonisolatedNonsendingyyYaF'
15+
// CHECK: bb0([[ACTOR:%.*]] : $Optional<any Actor>):
16+
// CHECK: retain_value [[ACTOR]]
17+
// CHECK: debug_value [[ACTOR]], let, name "iso"
18+
// CHECK: [[FUNC:%.*]] = function_ref @$s39isolated_nonsending_isolation_macro_sil4take3isoyScA_pSg_tF : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
19+
// CHECK: apply [[FUNC]]([[ACTOR]]) : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
20+
// CHECK: release_value [[ACTOR]]
21+
// CHECK: } // end sil function '$s39isolated_nonsending_isolation_macro_sil21nonisolatedNonsendingyyYaF'

‎test/Concurrency/nonisolated_nonsending_optimize_hoptoexecutor.swift‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,11 @@
33
// REQUIRES: concurrency
44

55
// CHECK-LABEL: sil hidden [noinline] @$s4testAAyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
6-
// CHECK: hop_to_executor
76
// CHECK: } // end sil function '$s4testAAyyYaF'
87
@inline(never)
98
nonisolated(nonsending) func test() async {}
109

1110
// CHECK-LABEL: sil hidden [noinline] @$s4test5test2yyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
12-
// CHECK: hop_to_executor
13-
// CHECK: hop_to_executor
1411
// CHECK: } // end sil function '$s4test5test2yyYaF'
1512
@inline(never)
1613
nonisolated(nonsending) func test2() async {
@@ -24,7 +21,6 @@ func test3() async {
2421
// CHECK-LABEL: sil @$s4test6calleryyYaF : $@convention(thin) @async () -> () {
2522
// CHECK: hop_to_executor
2623
// CHECK: function_ref @$s4testAAyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
27-
// CHECK: hop_to_executor
2824
// CHECK: function_ref @$s4test5test2yyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
2925
// CHECK: } // end sil function '$s4test6calleryyYaF'
3026
public func caller() async {

‎test/SILOptimizer/optimize_hop_to_executor.sil‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -393,8 +393,7 @@ bb0(%0 : @guaranteed $MyActor, %1 : @guaranteed $MyActor, %2 : @guaranteed $MyAc
393393
// CHECK-LABEL: sil [ossa] @callerIsolationInheritingStopsAllowsPropagating : $@convention(thin) @async (@guaranteed MyActor, @guaranteed MyActor, @guaranteed MyActor) -> () {
394394
// CHECK: bb0(
395395
// CHECK: hop_to_executor
396-
// CHECK: hop_to_executor
397-
// CHECK: hop_to_executor
396+
// CHECK-NOT: hop_to_executor
398397
// CHECK: } // end sil function 'callerIsolationInheritingStopsAllowsPropagating'
399398
sil [ossa] @callerIsolationInheritingStopsAllowsPropagating : $@convention(thin) @async (@guaranteed MyActor, @guaranteed MyActor, @guaranteed MyActor) -> () {
400399
bb0(%0 : @guaranteed $MyActor, %1 : @guaranteed $MyActor, %2 : @guaranteed $MyActor):
@@ -416,14 +415,13 @@ bb0(%0 : @guaranteed $MyActor, %1 : @guaranteed $MyActor, %2 : @guaranteed $MyAc
416415
// hop_to_executor due to ehre elase. We do eliminate one of the hop_to_executor
417416
// though.
418417
//
419-
// CHECK: sil [isolation "caller_isolation_inheriting"] [ossa] @callerIsolationInheritingStopsReturnDeadEnd : $@convention(thin) @async (@guaranteed MyActor, @guaranteed MyActor, @guaranteed MyActor) -> () {
418+
// CHECK: sil [isolation "caller_isolation_inheriting"] [ossa] @callerIsolationInheritingStopsReturnDeadEnd : $@convention(thin) @async (@sil_isolated @guaranteed MyActor) -> () {
420419
// CHECK: bb0(
421-
// CHECK-NEXT: hop_to_executor
422420
// CHECK-NEXT: tuple
423421
// CHECK-NEXT: return
424422
// CHECK: } // end sil function 'callerIsolationInheritingStopsReturnDeadEnd'
425-
sil [isolation "caller_isolation_inheriting"] [ossa] @callerIsolationInheritingStopsReturnDeadEnd : $@convention(thin) @async (@guaranteed MyActor, @guaranteed MyActor, @guaranteed MyActor) -> () {
426-
bb0(%0 : @guaranteed $MyActor, %1 : @guaranteed $MyActor, %2 : @guaranteed $MyActor):
423+
sil [isolation "caller_isolation_inheriting"] [ossa] @callerIsolationInheritingStopsReturnDeadEnd : $@convention(thin) @async (@sil_isolated @guaranteed MyActor) -> () {
424+
bb0(%0 : @guaranteed $MyActor):
427425
hop_to_executor %0
428426
hop_to_executor %0
429427
%9999 = tuple ()

0 commit comments

Comments
 (0)