Skip to content

Commit e18c68a

Browse files
gottesmmgottesmm
authored
Merge pull request #83083 from gottesmm/pr-e0e1571656b80b6c2d45f1a0cad842fbf6ae4800
[concurrency] Make optimize hop to executor more conservative for 6.2 around caller isolation inheriting functions.
2 parents 4f0cf9d + b394242 commit e18c68a

File tree

7 files changed

+323
-20
lines changed

7 files changed

+323
-20
lines changed

‎include/swift/SIL/ApplySite.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -894,6 +894,21 @@ class FullApplySite : public ApplySite {
894894
getNumIndirectSILErrorResults();
895895
}
896896

897+
std::optional<ActorIsolation> getActorIsolation() const {
898+
if (auto isolation = getIsolationCrossing();
899+
isolation && isolation->getCalleeIsolation())
900+
return isolation->getCalleeIsolation();
901+
auto *calleeFunction = getCalleeFunction();
902+
if (!calleeFunction)
903+
return {};
904+
return calleeFunction->getActorIsolation();
905+
}
906+
907+
bool isCallerIsolationInheriting() const {
908+
auto isolation = getActorIsolation();
909+
return isolation && isolation->isCallerIsolationInheriting();
910+
}
911+
897912
static FullApplySite getFromOpaqueValue(void *p) { return FullApplySite(p); }
898913

899914
static bool classof(const SILInstruction *inst) {

‎lib/SIL/IR/SILPrinter.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,11 @@ llvm::cl::opt<bool> SILPrintGenericSpecializationInfo(
9595
llvm::cl::desc("Include generic specialization"
9696
"information info in SIL output"));
9797

98+
llvm::cl::opt<bool> SILPrintFunctionIsolationInfo(
99+
"sil-print-function-isolation-info", llvm::cl::init(false),
100+
llvm::cl::desc("Print out isolation info on functions in a manner that SIL "
101+
"understands [e.x.: not in comments]"));
102+
98103
static std::string demangleSymbol(StringRef Name) {
99104
if (SILFullDemangle)
100105
return Demangle::demangleSymbolAsString(Name);
@@ -3630,6 +3635,15 @@ void SILFunction::print(SILPrintContext &PrintCtx) const {
36303635
OS << "[available " << availability.getVersionString() << "] ";
36313636
}
36323637

3638+
// This is here only for testing purposes.
3639+
if (SILPrintFunctionIsolationInfo) {
3640+
if (auto isolation = getActorIsolation()) {
3641+
OS << "[isolation \"";
3642+
isolation->printForSIL(OS);
3643+
OS << "\"] ";
3644+
}
3645+
}
3646+
36333647
switch (getInlineStrategy()) {
36343648
case NoInline: OS << "[noinline] "; break;
36353649
case AlwaysInline: OS << "[always_inline] "; break;

‎lib/SIL/Parser/ParseSIL.cpp‎

Lines changed: 45 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -680,11 +680,13 @@ static bool parseDeclSILOptional(
680680
SILFunction::Purpose *specialPurpose, Inline_t *inlineStrategy,
681681
OptimizationMode *optimizationMode, PerformanceConstraints *perfConstraints,
682682
bool *isPerformanceConstraint, bool *markedAsUsed, StringRef *section,
683-
bool *isLet, bool *isWeakImported, bool *needStackProtection, bool *isSpecialized,
684-
AvailabilityRange *availability, bool *isWithoutActuallyEscapingThunk,
683+
bool *isLet, bool *isWeakImported, bool *needStackProtection,
684+
bool *isSpecialized, AvailabilityRange *availability,
685+
bool *isWithoutActuallyEscapingThunk,
685686
SmallVectorImpl<std::string> *Semantics,
686687
SmallVectorImpl<ParsedSpecAttr> *SpecAttrs, ValueDecl **ClangDecl,
687-
EffectsKind *MRK, SILParser &SP, SILModule &M) {
688+
EffectsKind *MRK, ActorIsolation *actorIsolation, SILParser &SP,
689+
SILModule &M) {
688690
while (SP.P.consumeIf(tok::l_square)) {
689691
if (isLet && SP.P.Tok.is(tok::kw_let)) {
690692
*isLet = true;
@@ -785,7 +787,27 @@ static bool parseDeclSILOptional(
785787
*isPerformanceConstraint = true;
786788
else if (markedAsUsed && SP.P.Tok.getText() == "used")
787789
*markedAsUsed = true;
788-
else if (section && SP.P.Tok.getText() == "section") {
790+
else if (actorIsolation && SP.P.Tok.getText() == "isolation") {
791+
SP.P.consumeToken(tok::identifier);
792+
if (SP.P.Tok.getKind() != tok::string_literal) {
793+
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
794+
return true;
795+
}
796+
StringRef rawString = SP.P.Tok.getText().drop_front().drop_back();
797+
// TODO: By using a raw string here, we can perhaps put in a simple string
798+
// representation of an actor that can be parsed back. For now this is
799+
// just a quick hack so we can write tests.
800+
auto optIsolation = ActorIsolation::forSILString(
801+
SP.P.Context.getIdentifier(rawString).str());
802+
if (!optIsolation) {
803+
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
804+
return true;
805+
}
806+
*actorIsolation = *optIsolation;
807+
SP.P.consumeToken(tok::string_literal);
808+
SP.P.parseToken(tok::r_square, diag::expected_in_attribute_list);
809+
continue;
810+
} else if (section && SP.P.Tok.getText() == "section") {
789811
SP.P.consumeToken(tok::identifier);
790812
if (SP.P.Tok.getKind() != tok::string_literal) {
791813
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
@@ -799,8 +821,7 @@ static bool parseDeclSILOptional(
799821

800822
SP.P.parseToken(tok::r_square, diag::expected_in_attribute_list);
801823
continue;
802-
}
803-
else if (inlineStrategy && SP.P.Tok.getText() == "always_inline")
824+
} else if (inlineStrategy && SP.P.Tok.getText() == "always_inline")
804825
*inlineStrategy = AlwaysInline;
805826
else if (MRK && SP.P.Tok.getText() == "readnone")
806827
*MRK = EffectsKind::ReadNone;
@@ -7320,6 +7341,7 @@ bool SILParserState::parseDeclSIL(Parser &P) {
73207341
SILFunction *DynamicallyReplacedFunction = nullptr;
73217342
SILFunction *AdHocWitnessFunction = nullptr;
73227343
Identifier objCReplacementFor;
7344+
ActorIsolation actorIsolation;
73237345
if (parseSILLinkage(FnLinkage, P) ||
73247346
parseDeclSILOptional(
73257347
&isTransparent, &isSerialized, &isCanonical, &hasOwnershipSSA,
@@ -7330,8 +7352,9 @@ bool SILParserState::parseDeclSIL(Parser &P) {
73307352
&objCReplacementFor, &specialPurpose, &inlineStrategy,
73317353
&optimizationMode, &perfConstr, &isPerformanceConstraint,
73327354
&markedAsUsed, &section, nullptr, &isWeakImported,
7333-
&needStackProtection, nullptr, &availability, &isWithoutActuallyEscapingThunk,
7334-
&Semantics, &SpecAttrs, &ClangDecl, &MRK, FunctionState, M) ||
7355+
&needStackProtection, nullptr, &availability,
7356+
&isWithoutActuallyEscapingThunk, &Semantics, &SpecAttrs, &ClangDecl,
7357+
&MRK, &actorIsolation, FunctionState, M) ||
73357358
P.parseToken(tok::at_sign, diag::expected_sil_function_name) ||
73367359
P.parseIdentifier(FnName, FnNameLoc, /*diagnoseDollarPrefix=*/false,
73377360
diag::expected_sil_function_name) ||
@@ -7391,6 +7414,8 @@ bool SILParserState::parseDeclSIL(Parser &P) {
73917414
for (auto &Attr : Semantics) {
73927415
FunctionState.F->addSemanticsAttr(Attr);
73937416
}
7417+
if (actorIsolation)
7418+
FunctionState.F->setActorIsolation(actorIsolation);
73947419
// Now that we have a SILFunction parse the body, if present.
73957420

73967421
bool isDefinition = false;
@@ -7580,11 +7605,11 @@ bool SILParserState::parseSILGlobal(Parser &P) {
75807605

75817606
SILParser State(P);
75827607
if (parseSILLinkage(GlobalLinkage, P) ||
7583-
parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr,
7608+
parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr, nullptr,
75847609
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
75857610
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
7611+
nullptr, nullptr, nullptr, nullptr, nullptr, &isLet,
75867612
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
7587-
&isLet, nullptr, nullptr, nullptr, nullptr, nullptr,
75887613
nullptr, nullptr, nullptr, nullptr, State, M) ||
75897614
P.parseToken(tok::at_sign, diag::expected_sil_value_name) ||
75907615
P.parseIdentifier(GlobalName, NameLoc, /*diagnoseDollarPrefix=*/false,
@@ -7638,7 +7663,7 @@ bool SILParserState::parseSILProperty(Parser &P) {
76387663
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
76397664
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
76407665
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
7641-
nullptr, nullptr, nullptr, SP, M))
7666+
nullptr, nullptr, nullptr, nullptr, SP, M))
76427667
return true;
76437668

76447669
ValueDecl *VD;
@@ -7708,7 +7733,7 @@ bool SILParserState::parseSILVTable(Parser &P) {
77087733
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
77097734
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
77107735
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
7711-
nullptr, nullptr, nullptr, VTableState, M))
7736+
nullptr, nullptr, nullptr, nullptr, VTableState, M))
77127737
return true;
77137738

77147739

@@ -7831,7 +7856,8 @@ bool SILParserState::parseSILMoveOnlyDeinit(Parser &parser) {
78317856
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
78327857
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
78337858
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
7834-
nullptr, nullptr, nullptr, moveOnlyDeinitTableState, M))
7859+
nullptr, nullptr, nullptr, nullptr,
7860+
moveOnlyDeinitTableState, M))
78357861
return true;
78367862

78377863
// Parse the class name.
@@ -8371,12 +8397,12 @@ bool SILParserState::parseSILWitnessTable(Parser &P) {
83718397

83728398
SerializedKind_t isSerialized = IsNotSerialized;
83738399
bool isSpecialized = false;
8374-
if (parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr, nullptr,
8375-
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
8376-
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
8377-
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
8378-
nullptr, nullptr, &isSpecialized, nullptr, nullptr, nullptr,
8379-
nullptr, nullptr, nullptr, WitnessState, M))
8400+
if (parseDeclSILOptional(
8401+
nullptr, &isSerialized, nullptr, nullptr, nullptr, nullptr, nullptr,
8402+
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
8403+
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
8404+
nullptr, nullptr, nullptr, nullptr, &isSpecialized, nullptr, nullptr,
8405+
nullptr, nullptr, nullptr, nullptr, nullptr, WitnessState, M))
83808406
return true;
83818407

83828408
// Parse the protocol conformance.

‎lib/SILOptimizer/Mandatory/OptimizeHopToExecutor.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,12 +193,16 @@ void OptimizeHopToExecutor::solveDataflowBackward() {
193193
/// Returns true if \p inst is a suspension point or an async call.
194194
static bool isSuspensionPoint(SILInstruction *inst) {
195195
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.
196199
if (applySite.isAsync())
197200
return true;
198201
return false;
199202
}
200203
if (isa<AwaitAsyncContinuationInst>(inst))
201204
return true;
205+
202206
return false;
203207
}
204208

@@ -311,6 +315,40 @@ void OptimizeHopToExecutor::updateNeedExecutor(int &needExecutor,
311315
needExecutor = BlockState::NoExecutorNeeded;
312316
return;
313317
}
318+
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 (auto isolation = inst->getFunction()->getActorIsolation();
346+
isolation && isolation->isCallerIsolationInheriting() &&
347+
isa<ReturnInst>(inst)) {
348+
needExecutor = BlockState::ExecutorNeeded;
349+
return;
350+
}
351+
314352
if (isSuspensionPoint(inst)) {
315353
needExecutor = BlockState::NoExecutorNeeded;
316354
return;

‎test/Concurrency/nonisolated_nonsending_optimize_hoptoexecutor.swift‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// RUN: %target-swift-frontend -module-name test -swift-version 6 -emit-sil %s | %FileCheck --implicit-check-not=hop_to_executor %s
2+
3+
// REQUIRES: concurrency
4+
5+
// CHECK-LABEL: sil hidden [noinline] @$s4testAAyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
6+
// CHECK: hop_to_executor
7+
// CHECK: } // end sil function '$s4testAAyyYaF'
8+
@inline(never)
9+
nonisolated(nonsending) func test() async {}
10+
11+
// 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
14+
// CHECK: } // end sil function '$s4test5test2yyYaF'
15+
@inline(never)
16+
nonisolated(nonsending) func test2() async {
17+
await test()
18+
}
19+
20+
@inline(never)
21+
func test3() async {
22+
}
23+
24+
// CHECK-LABEL: sil @$s4test6calleryyYaF : $@convention(thin) @async () -> () {
25+
// CHECK: hop_to_executor
26+
// CHECK: function_ref @$s4testAAyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
27+
// CHECK: hop_to_executor
28+
// CHECK: function_ref @$s4test5test2yyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
29+
// CHECK: } // end sil function '$s4test6calleryyYaF'
30+
public func caller() async {
31+
await test()
32+
await test2()
33+
await test3()
34+
}

‎test/SIL/Parser/actor_isolation.sil‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// RUN: %target-sil-opt -sil-print-function-isolation-info -enable-objc-interop -enable-sil-verify-all=true %s | %target-sil-opt -sil-print-function-isolation-info -enable-objc-interop -enable-sil-verify-all=true | %FileCheck %s
2+
3+
// REQUIRES: asserts
4+
5+
// CHECK: // func_with_caller_isolation_inheriting
6+
// CHECK: // Isolation: caller_isolation_inheriting
7+
// CHECK: sil [isolation "caller_isolation_inheriting"] @func_with_caller_isolation_inheriting : $@convention(thin) () -> () {
8+
sil [isolation "caller_isolation_inheriting"] @func_with_caller_isolation_inheriting : $@convention(thin) () -> () {
9+
bb0:
10+
%0 = tuple ()
11+
return %0 : $()
12+
}
13+
14+
// CHECK: // func_with_global_actor_isolation
15+
// CHECK: // Isolation: global_actor. type: <null>
16+
// CHECK: sil [isolation "global_actor"] @func_with_global_actor_isolation : $@convention(thin) () -> () {
17+
sil [isolation "global_actor"] @func_with_global_actor_isolation : $@convention(thin) () -> () {
18+
bb0:
19+
%0 = tuple ()
20+
return %0 : $()
21+
}
22+
23+
// CHECK: // func_with_actor_instance_isolation
24+
// CHECK: // Isolation: actor_instance
25+
// CHECK: sil [isolation "actor_instance"] @func_with_actor_instance_isolation : $@convention(thin) () -> () {
26+
sil [isolation "actor_instance"] @func_with_actor_instance_isolation : $@convention(thin) () -> () {
27+
bb0:
28+
%0 = tuple ()
29+
return %0 : $()
30+
}
31+
32+
// CHECK: // func_with_nonisolated_isolation
33+
// CHECK: // Isolation: nonisolated
34+
// CHECK: sil [isolation "nonisolated"] @func_with_nonisolated_isolation : $@convention(thin) () -> () {
35+
sil [isolation "nonisolated"] @func_with_nonisolated_isolation : $@convention(thin) () -> () {
36+
bb0:
37+
%0 = tuple ()
38+
return %0 : $()
39+
}

0 commit comments

Comments
 (0)