Skip to content

Commit 8243b5c

Browse files
committed
[region-isolation] If a value is dynamically actor isolated, do not consider it transferred if the transfer statically was to that same actor isolation.
This issue can come up when a value is initially statically disconnected, but after we performed dataflow, we discovered that it was actually actor isolated at the transfer point, implying that we are not actually transferring. Example: ```swift @mainactor func testGlobalAndGlobalIsolatedPartialApplyMatch2() { var ns = (NonSendableKlass(), NonSendableKlass()) // Regions: (ns.0, ns.1), {(mainActorIsolatedGlobal), @mainactor} ns.0 = mainActorIsolatedGlobal // Regions: {(ns.0, ns.1, mainActorIsolatedGlobal), @mainactor} // This is not a transfer since ns is already main actor isolated. let _ = { @mainactor in print(ns) } useValue(ns) } ``` To do this, I also added to SILFunction an actor isolation that SILGen puts on the SILFunction during pre function visitation. We don't print it or serialize it for now. rdar://123474616
1 parent d0d2f2d commit 8243b5c

File tree

8 files changed

+110
-17
lines changed

8 files changed

+110
-17
lines changed

include/swift/SIL/SILFunction.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,9 @@ class SILFunction
347347
/// block indices.
348348
unsigned BlockListChangeIdx = 0;
349349

350+
/// The isolation of this function.
351+
std::optional<ActorIsolation> actorIsolation;
352+
350353
/// The function's bare attribute. Bare means that the function is SIL-only
351354
/// and does not require debug info.
352355
unsigned Bare : 1;
@@ -1367,6 +1370,14 @@ class SILFunction
13671370
return false;
13681371
}
13691372

1373+
void setActorIsolation(ActorIsolation newActorIsolation) {
1374+
actorIsolation = newActorIsolation;
1375+
}
1376+
1377+
std::optional<ActorIsolation> getActorIsolation() const {
1378+
return actorIsolation;
1379+
}
1380+
13701381
//===--------------------------------------------------------------------===//
13711382
// Block List Access
13721383
//===--------------------------------------------------------------------===//

include/swift/SILOptimizer/Utils/PartitionUtils.h

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ class SILIsolationInfo {
204204
return getActorIsolated(actorIsolation);
205205
if (nomDecl->isActor())
206206
return {Kind::Actor, nomDecl};
207-
return {};
207+
return SILIsolationInfo();
208208
}
209209

210210
static SILIsolationInfo getTaskIsolated(SILValue value) {
@@ -789,6 +789,21 @@ struct PartitionOpEvaluator {
789789
std::tie(transferredRegionIsolation, isClosureCapturedElt) =
790790
getIsolationRegionInfo(transferredRegion, op.getSourceOp());
791791

792+
// Before we do anything, see if our dynamic isolation kind is the same as
793+
// the isolation info for our partition op. If they match, this is not a
794+
// real transfer operation.
795+
//
796+
// DISCUSSION: We couldn't not emit this earlier since we needed the
797+
// dynamic isolation info of our value.
798+
if (transferredRegionIsolation.isActorIsolated()) {
799+
if (auto calleeIsolationInfo =
800+
SILIsolationInfo::get(op.getSourceInst())) {
801+
if (transferredRegionIsolation == calleeIsolationInfo) {
802+
return;
803+
}
804+
}
805+
}
806+
792807
// If we merged anything, we need to handle a transfer
793808
// non-transferrable. We pass in the dynamic isolation region info of our
794809
// region.

lib/SILGen/SILGen.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1217,6 +1217,10 @@ void SILGenModule::preEmitFunction(SILDeclRef constant, SILFunction *F,
12171217
if (F->getLoweredFunctionType()->isPolymorphic())
12181218
F->setGenericEnvironment(Types.getConstantGenericEnvironment(constant));
12191219

1220+
// Set the actor isolation of the function to its innermost decl context.
1221+
F->setActorIsolation(
1222+
getActorIsolationOfContext(constant.getInnermostDeclContext()));
1223+
12201224
// Create a debug scope for the function using astNode as source location.
12211225
F->setDebugScope(new (M) SILDebugScope(Loc, F));
12221226

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1470,6 +1470,7 @@ class PartitionOpTranslator {
14701470
llvm::SmallVector<Element, 8> nonSendableJoinedIndices;
14711471
llvm::SmallVector<Element, 8> nonSendableSeparateIndices;
14721472
for (SILArgument *arg : functionArguments) {
1473+
// This will decide what the isolation region is.
14731474
if (auto state = tryToTrackValue(arg)) {
14741475
// If we can transfer our parameter, just add it to
14751476
// nonSendableSeparateIndices.
@@ -1485,9 +1486,8 @@ class PartitionOpTranslator {
14851486

14861487
// Otherwise, it is one of our merged parameters. Add it to the never
14871488
// transfer list and to the region join list.
1488-
LLVM_DEBUG(llvm::dbgs() << " %%" << state->getID() << ": " << *arg);
1489-
valueMap.mergeIsolationRegionInfo(
1490-
arg, SILIsolationInfo::getTaskIsolated(arg));
1489+
LLVM_DEBUG(llvm::dbgs() << " %%" << state->getID() << ": ";
1490+
state->print(llvm::dbgs()); llvm::dbgs() << *arg);
14911491
nonSendableJoinedIndices.push_back(state->getID());
14921492
}
14931493
}

lib/SILOptimizer/Utils/PartitionUtils.cpp

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,21 @@ static llvm::cl::opt<bool, true> // The parser
4242
//===----------------------------------------------------------------------===//
4343

4444
SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
45-
if (ApplyExpr *apply = inst->getLoc().getAsASTNode<ApplyExpr>())
46-
if (auto crossing = apply->getIsolationCrossing())
47-
return SILIsolationInfo::getActorIsolated(crossing->getCalleeIsolation());
45+
if (ApplyExpr *apply = inst->getLoc().getAsASTNode<ApplyExpr>()) {
46+
if (auto crossing = apply->getIsolationCrossing()) {
47+
if (crossing->getCalleeIsolation().isActorIsolated())
48+
return SILIsolationInfo::getActorIsolated(
49+
crossing->getCalleeIsolation());
50+
}
51+
}
4852

4953
if (auto fas = FullApplySite::isa(inst)) {
50-
if (auto crossing = fas.getIsolationCrossing())
51-
return SILIsolationInfo::getActorIsolated(crossing->getCalleeIsolation());
54+
if (auto crossing = fas.getIsolationCrossing()) {
55+
if (crossing->getCalleeIsolation().isActorIsolated()) {
56+
return SILIsolationInfo::getActorIsolated(
57+
crossing->getCalleeIsolation());
58+
}
59+
}
5260

5361
if (fas.hasSelfArgument()) {
5462
auto &self = fas.getSelfArgumentOperand();
@@ -79,9 +87,18 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
7987
}
8088

8189
SILIsolationInfo SILIsolationInfo::get(SILFunctionArgument *arg) {
90+
// If we have self and our function is actor isolated, all of our arguments
91+
// should be marked as actor isolated.
8292
if (auto *self = arg->getFunction()->maybeGetSelfArgument()) {
83-
if (auto *nomDecl = self->getType().getNominalOrBoundGenericNominal()) {
84-
return SILIsolationInfo::getActorIsolated(nomDecl);
93+
if (auto functionIsolation = arg->getFunction()->getActorIsolation()) {
94+
if (functionIsolation->isActorIsolated()) {
95+
if (auto *nomDecl = self->getType().getNominalOrBoundGenericNominal()) {
96+
if (auto isolationInfo =
97+
SILIsolationInfo::getActorIsolated(nomDecl)) {
98+
return isolationInfo;
99+
}
100+
}
101+
}
85102
}
86103
}
87104

@@ -171,7 +188,7 @@ bool SILIsolationInfo::operator==(const SILIsolationInfo &other) const {
171188

172189
// Otherwise, try to use the inferred actor decl.
173190
auto *lhsDecl = tryInferActorDecl();
174-
auto *rhsDecl = tryInferActorDecl();
191+
auto *rhsDecl = other.tryInferActorDecl();
175192
if (lhsDecl && rhsDecl)
176193
return lhsDecl == rhsDecl;
177194

test/Concurrency/transfernonsendable_global_actor.swift

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,21 @@ struct Clock {
190190
@MainActor func testGlobalAndGlobalIsolatedPartialApplyMatch() {
191191
let ns = mainActorIsolatedGlobal
192192

193-
// TODO: We should not consider this to be a transfer.
193+
// This is not a transfer since ns is already main actor isolated.
194194
let _ = { @MainActor in
195-
print(ns) // expected-tns-warning {{transferring 'ns' may cause a race}}
196-
// expected-tns-note @-1 {{main actor-isolated 'ns' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
195+
print(ns)
196+
}
197+
198+
useValue(ns)
199+
}
200+
201+
@MainActor func testGlobalAndGlobalIsolatedPartialApplyMatch2() {
202+
var ns = (NonSendableKlass(), NonSendableKlass())
203+
ns.0 = mainActorIsolatedGlobal
204+
205+
// This is not a transfer since ns is already main actor isolated.
206+
let _ = { @MainActor in
207+
print(ns)
197208
}
198209

199210
useValue(ns)
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// RUN: %target-swift-frontend -emit-sil -swift-version 6 -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults -verify %s -o /dev/null -parse-as-library
2+
3+
// README: Once we loosen the parser so that transferring is rejected in Sema
4+
// instead of the parser, move into the normal
5+
// transfernonsendable_global_actor.swift
6+
7+
////////////////////////
8+
// MARK: Declarations //
9+
////////////////////////
10+
11+
class NonSendableKlass {}
12+
13+
extension Task where Failure == Never {
14+
public static func fakeInit(
15+
@_implicitSelfCapture operation: transferring @escaping () async -> Success
16+
) {}
17+
}
18+
19+
func useValue<T>(_ t: T) {}
20+
21+
/////////////////
22+
// MARK: Tests //
23+
/////////////////
24+
25+
@MainActor func testGlobalFakeInit() {
26+
let ns = NonSendableKlass()
27+
28+
// Will be resolved once @MainActor is @Sendable
29+
Task.fakeInit { @MainActor in // expected-error {{main actor-isolated value of type '@MainActor () async -> ()' passed as a strongly transferred parameter}}
30+
print(ns) // expected-error {{transferring 'ns' may cause a race}}
31+
// expected-note @-1 {{disconnected 'ns' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
32+
}
33+
34+
useValue(ns) // expected-note {{use here could race}}
35+
}

test/Concurrency/transfernonsendable_isolationcrossing_partialapply.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ actor ProtectsNonSendable {
4141
// isolated since this is nonisolated.
4242
self.assumeIsolated { isolatedSelf in
4343
isolatedSelf.ns = nsArg // expected-warning {{transferring 'nsArg' may cause a race}}
44-
// expected-note @-1 {{actor-isolated 'nsArg' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses}}
44+
// expected-note @-1 {{task-isolated 'nsArg' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses}}
4545
}
4646
}
4747

@@ -50,7 +50,7 @@ actor ProtectsNonSendable {
5050
doSomething(l, nsArg)
5151
self.assumeIsolated { isolatedSelf in
5252
isolatedSelf.ns = l // expected-warning {{transferring 'l' may cause a race}}
53-
// expected-note @-1 {{actor-isolated 'l' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses}}
53+
// expected-note @-1 {{task-isolated 'l' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses}}
5454
}
5555
}
5656

0 commit comments

Comments
 (0)