Skip to content

[concurrency] Make optimize hop to executor more conservative for 6.2 around caller isolation inheriting functions. #83083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions include/swift/SIL/ApplySite.h
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,21 @@ class FullApplySite : public ApplySite {
getNumIndirectSILErrorResults();
}

std::optional<ActorIsolation> getActorIsolation() const {
if (auto isolation = getIsolationCrossing();
isolation && isolation->getCalleeIsolation())
return isolation->getCalleeIsolation();
auto *calleeFunction = getCalleeFunction();
if (!calleeFunction)
return {};
return calleeFunction->getActorIsolation();
}

bool isCallerIsolationInheriting() const {
auto isolation = getActorIsolation();
return isolation && isolation->isCallerIsolationInheriting();
}

static FullApplySite getFromOpaqueValue(void *p) { return FullApplySite(p); }

static bool classof(const SILInstruction *inst) {
Expand Down
14 changes: 14 additions & 0 deletions lib/SIL/IR/SILPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ llvm::cl::opt<bool> SILPrintGenericSpecializationInfo(
llvm::cl::desc("Include generic specialization"
"information info in SIL output"));

llvm::cl::opt<bool> SILPrintFunctionIsolationInfo(
"sil-print-function-isolation-info", llvm::cl::init(false),
llvm::cl::desc("Print out isolation info on functions in a manner that SIL "
"understands [e.x.: not in comments]"));

static std::string demangleSymbol(StringRef Name) {
if (SILFullDemangle)
return Demangle::demangleSymbolAsString(Name);
Expand Down Expand Up @@ -3630,6 +3635,15 @@ void SILFunction::print(SILPrintContext &PrintCtx) const {
OS << "[available " << availability.getVersionString() << "] ";
}

// This is here only for testing purposes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it just for "testing"? Seems like would be useful to just keep around anyway right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I am only doing it for testing is that I am going to revamp this post 6.2. I don't want people to rely on it.

if (SILPrintFunctionIsolationInfo) {
if (auto isolation = getActorIsolation()) {
OS << "[isolation \"";
isolation->printForSIL(OS);
OS << "\"] ";
}
}

switch (getInlineStrategy()) {
case NoInline: OS << "[noinline] "; break;
case AlwaysInline: OS << "[always_inline] "; break;
Expand Down
64 changes: 45 additions & 19 deletions lib/SIL/Parser/ParseSIL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -680,11 +680,13 @@ static bool parseDeclSILOptional(
SILFunction::Purpose *specialPurpose, Inline_t *inlineStrategy,
OptimizationMode *optimizationMode, PerformanceConstraints *perfConstraints,
bool *isPerformanceConstraint, bool *markedAsUsed, StringRef *section,
bool *isLet, bool *isWeakImported, bool *needStackProtection, bool *isSpecialized,
AvailabilityRange *availability, bool *isWithoutActuallyEscapingThunk,
bool *isLet, bool *isWeakImported, bool *needStackProtection,
bool *isSpecialized, AvailabilityRange *availability,
bool *isWithoutActuallyEscapingThunk,
SmallVectorImpl<std::string> *Semantics,
SmallVectorImpl<ParsedSpecAttr> *SpecAttrs, ValueDecl **ClangDecl,
EffectsKind *MRK, SILParser &SP, SILModule &M) {
EffectsKind *MRK, ActorIsolation *actorIsolation, SILParser &SP,
SILModule &M) {
while (SP.P.consumeIf(tok::l_square)) {
if (isLet && SP.P.Tok.is(tok::kw_let)) {
*isLet = true;
Expand Down Expand Up @@ -785,7 +787,27 @@ static bool parseDeclSILOptional(
*isPerformanceConstraint = true;
else if (markedAsUsed && SP.P.Tok.getText() == "used")
*markedAsUsed = true;
else if (section && SP.P.Tok.getText() == "section") {
else if (actorIsolation && SP.P.Tok.getText() == "isolation") {
SP.P.consumeToken(tok::identifier);
if (SP.P.Tok.getKind() != tok::string_literal) {
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
return true;
}
StringRef rawString = SP.P.Tok.getText().drop_front().drop_back();
// TODO: By using a raw string here, we can perhaps put in a simple string
// representation of an actor that can be parsed back. For now this is
// just a quick hack so we can write tests.
auto optIsolation = ActorIsolation::forSILString(
SP.P.Context.getIdentifier(rawString).str());
if (!optIsolation) {
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
return true;
}
*actorIsolation = *optIsolation;
SP.P.consumeToken(tok::string_literal);
SP.P.parseToken(tok::r_square, diag::expected_in_attribute_list);
continue;
} else if (section && SP.P.Tok.getText() == "section") {
SP.P.consumeToken(tok::identifier);
if (SP.P.Tok.getKind() != tok::string_literal) {
SP.P.diagnose(SP.P.Tok, diag::expected_in_attribute_list);
Expand All @@ -799,8 +821,7 @@ static bool parseDeclSILOptional(

SP.P.parseToken(tok::r_square, diag::expected_in_attribute_list);
continue;
}
else if (inlineStrategy && SP.P.Tok.getText() == "always_inline")
} else if (inlineStrategy && SP.P.Tok.getText() == "always_inline")
*inlineStrategy = AlwaysInline;
else if (MRK && SP.P.Tok.getText() == "readnone")
*MRK = EffectsKind::ReadNone;
Expand Down Expand Up @@ -7320,6 +7341,7 @@ bool SILParserState::parseDeclSIL(Parser &P) {
SILFunction *DynamicallyReplacedFunction = nullptr;
SILFunction *AdHocWitnessFunction = nullptr;
Identifier objCReplacementFor;
ActorIsolation actorIsolation;
if (parseSILLinkage(FnLinkage, P) ||
parseDeclSILOptional(
&isTransparent, &isSerialized, &isCanonical, &hasOwnershipSSA,
Expand All @@ -7330,8 +7352,9 @@ bool SILParserState::parseDeclSIL(Parser &P) {
&objCReplacementFor, &specialPurpose, &inlineStrategy,
&optimizationMode, &perfConstr, &isPerformanceConstraint,
&markedAsUsed, &section, nullptr, &isWeakImported,
&needStackProtection, nullptr, &availability, &isWithoutActuallyEscapingThunk,
&Semantics, &SpecAttrs, &ClangDecl, &MRK, FunctionState, M) ||
&needStackProtection, nullptr, &availability,
&isWithoutActuallyEscapingThunk, &Semantics, &SpecAttrs, &ClangDecl,
&MRK, &actorIsolation, FunctionState, M) ||
P.parseToken(tok::at_sign, diag::expected_sil_function_name) ||
P.parseIdentifier(FnName, FnNameLoc, /*diagnoseDollarPrefix=*/false,
diag::expected_sil_function_name) ||
Expand Down Expand Up @@ -7391,6 +7414,8 @@ bool SILParserState::parseDeclSIL(Parser &P) {
for (auto &Attr : Semantics) {
FunctionState.F->addSemanticsAttr(Attr);
}
if (actorIsolation)
FunctionState.F->setActorIsolation(actorIsolation);
// Now that we have a SILFunction parse the body, if present.

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

SILParser State(P);
if (parseSILLinkage(GlobalLinkage, P) ||
parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr,
parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, &isLet,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
&isLet, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, State, M) ||
P.parseToken(tok::at_sign, diag::expected_sil_value_name) ||
P.parseIdentifier(GlobalName, NameLoc, /*diagnoseDollarPrefix=*/false,
Expand Down Expand Up @@ -7638,7 +7663,7 @@ bool SILParserState::parseSILProperty(Parser &P) {
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, SP, M))
nullptr, nullptr, nullptr, nullptr, SP, M))
return true;

ValueDecl *VD;
Expand Down Expand Up @@ -7708,7 +7733,7 @@ bool SILParserState::parseSILVTable(Parser &P) {
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, VTableState, M))
nullptr, nullptr, nullptr, nullptr, VTableState, M))
return true;


Expand Down Expand Up @@ -7831,7 +7856,8 @@ bool SILParserState::parseSILMoveOnlyDeinit(Parser &parser) {
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, moveOnlyDeinitTableState, M))
nullptr, nullptr, nullptr, nullptr,
moveOnlyDeinitTableState, M))
return true;

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

SerializedKind_t isSerialized = IsNotSerialized;
bool isSpecialized = false;
if (parseDeclSILOptional(nullptr, &isSerialized, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, &isSpecialized, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, WitnessState, M))
if (parseDeclSILOptional(
nullptr, &isSerialized, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, &isSpecialized, nullptr, nullptr,
nullptr, nullptr, nullptr, nullptr, nullptr, WitnessState, M))
return true;

// Parse the protocol conformance.
Expand Down
38 changes: 38 additions & 0 deletions lib/SILOptimizer/Mandatory/OptimizeHopToExecutor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,16 @@ void OptimizeHopToExecutor::solveDataflowBackward() {
/// Returns true if \p inst is a suspension point or an async call.
static bool isSuspensionPoint(SILInstruction *inst) {
if (auto applySite = FullApplySite::isa(inst)) {
// NOTE: For 6.2, we consider nonisolated(nonsending) to be a suspension
// point, when it really isn't. We do this so that we have a truly
// conservative change that does not change output.
if (applySite.isAsync())
return true;
return false;
}
if (isa<AwaitAsyncContinuationInst>(inst))
return true;

return false;
}

Expand Down Expand Up @@ -311,6 +315,40 @@ void OptimizeHopToExecutor::updateNeedExecutor(int &needExecutor,
needExecutor = BlockState::NoExecutorNeeded;
return;
}

// For 6.2 to be conservative, if we are calling a function with
// caller_isolation_inheriting isolation, treat the callsite as if the
// callsite is an instruction that needs an executor.
//
// DISCUSSION: The reason why we are doing this is that in 6.2, we are going
// to continue treating caller isolation inheriting functions as a suspension
// point for the purpose of eliminating redundant hop to executor to not make
// this optimization more aggressive. Post 6.2, we will stop treating caller
// isolation inheriting functions as suspension points, meaning this code can
// be deleted.
if (auto fas = FullApplySite::isa(inst);
fas && fas.isAsync() && fas.isCallerIsolationInheriting()) {
needExecutor = BlockState::ExecutorNeeded;
return;
}

// For 6.2, if we are in a caller isolation inheriting function, we need to
// treat its return as an executor needing function before
// isSuspensionPoint.
//
// DISCUSSION: We need to do this here since for 6.2, a caller isolation
// inheriting function is going to be considered a suspension point to be
// conservative and make this optimization strictly more conservative. Post
// 6.2, since caller isolation inheriting functions will no longer be
// considered suspension points, we will be able to sink this code into needs
// executor.
if (auto isolation = inst->getFunction()->getActorIsolation();
isolation && isolation->isCallerIsolationInheriting() &&
isa<ReturnInst>(inst)) {
needExecutor = BlockState::ExecutorNeeded;
return;
}

if (isSuspensionPoint(inst)) {
needExecutor = BlockState::NoExecutorNeeded;
return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// RUN: %target-swift-frontend -module-name test -swift-version 6 -emit-sil %s | %FileCheck --implicit-check-not=hop_to_executor %s

// REQUIRES: concurrency

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

// CHECK-LABEL: sil hidden [noinline] @$s4test5test2yyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
// CHECK: hop_to_executor
// CHECK: hop_to_executor
// CHECK: } // end sil function '$s4test5test2yyYaF'
@inline(never)
nonisolated(nonsending) func test2() async {
await test()
}

@inline(never)
func test3() async {
}

// CHECK-LABEL: sil @$s4test6calleryyYaF : $@convention(thin) @async () -> () {
// CHECK: hop_to_executor
// CHECK: function_ref @$s4testAAyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
// CHECK: hop_to_executor
// CHECK: function_ref @$s4test5test2yyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
// CHECK: } // end sil function '$s4test6calleryyYaF'
public func caller() async {
await test()
await test2()
await test3()
}
39 changes: 39 additions & 0 deletions test/SIL/Parser/actor_isolation.sil
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// 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

// REQUIRES: asserts

// CHECK: // func_with_caller_isolation_inheriting
// CHECK: // Isolation: caller_isolation_inheriting
// CHECK: sil [isolation "caller_isolation_inheriting"] @func_with_caller_isolation_inheriting : $@convention(thin) () -> () {
sil [isolation "caller_isolation_inheriting"] @func_with_caller_isolation_inheriting : $@convention(thin) () -> () {
bb0:
%0 = tuple ()
return %0 : $()
}

// CHECK: // func_with_global_actor_isolation
// CHECK: // Isolation: global_actor. type: <null>
// CHECK: sil [isolation "global_actor"] @func_with_global_actor_isolation : $@convention(thin) () -> () {
sil [isolation "global_actor"] @func_with_global_actor_isolation : $@convention(thin) () -> () {
bb0:
%0 = tuple ()
return %0 : $()
}

// CHECK: // func_with_actor_instance_isolation
// CHECK: // Isolation: actor_instance
// CHECK: sil [isolation "actor_instance"] @func_with_actor_instance_isolation : $@convention(thin) () -> () {
sil [isolation "actor_instance"] @func_with_actor_instance_isolation : $@convention(thin) () -> () {
bb0:
%0 = tuple ()
return %0 : $()
}

// CHECK: // func_with_nonisolated_isolation
// CHECK: // Isolation: nonisolated
// CHECK: sil [isolation "nonisolated"] @func_with_nonisolated_isolation : $@convention(thin) () -> () {
sil [isolation "nonisolated"] @func_with_nonisolated_isolation : $@convention(thin) () -> () {
bb0:
%0 = tuple ()
return %0 : $()
}
Loading