Skip to content

Commit e36aab6

Browse files
gottesmmgottesmm
committed
[region-isolation] Squelch use after transfer if the use has effectively the same isolation as the transfer inst.
To be more specific this means that either: 1. The use is actually isolated to the same actor. This could mean that the use is global actor isolated to the same function. 2. The use is nonisolated but is executing within a function that is globally isolated to the same isolation domain. rdar://123474616
1 parent 8243b5c commit e36aab6

File tree

5 files changed

+99
-25
lines changed

5 files changed

+99
-25
lines changed

‎include/swift/SILOptimizer/Utils/PartitionUtils.h‎

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "swift/Basic/FrozenMultiMap.h"
1818
#include "swift/Basic/ImmutablePointerSet.h"
1919
#include "swift/Basic/LLVM.h"
20+
#include "swift/SIL/SILFunction.h"
2021
#include "swift/SIL/SILInstruction.h"
2122
#include "llvm/ADT/SmallVector.h"
2223
#include "llvm/Support/Debug.h"
@@ -756,8 +757,8 @@ struct PartitionOpEvaluator {
756757
// value... emit an error.
757758
if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) {
758759
for (auto transferredOperand : transferredOperandSet->data()) {
759-
handleLocalUseAfterTransfer(op, op.getOpArgs()[1],
760-
transferredOperand);
760+
handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1],
761+
transferredOperand);
761762
}
762763
}
763764
p.assignElement(op.getOpArgs()[0], op.getOpArgs()[1]);
@@ -839,14 +840,14 @@ struct PartitionOpEvaluator {
839840
// if attempting to merge a transferred region, handle the failure
840841
if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[0])) {
841842
for (auto transferredOperand : transferredOperandSet->data()) {
842-
handleLocalUseAfterTransfer(op, op.getOpArgs()[0],
843-
transferredOperand);
843+
handleLocalUseAfterTransferHelper(op, op.getOpArgs()[0],
844+
transferredOperand);
844845
}
845846
}
846847
if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) {
847848
for (auto transferredOperand : transferredOperandSet->data()) {
848-
handleLocalUseAfterTransfer(op, op.getOpArgs()[1],
849-
transferredOperand);
849+
handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1],
850+
transferredOperand);
850851
}
851852
}
852853

@@ -859,8 +860,8 @@ struct PartitionOpEvaluator {
859860
"Require PartitionOp's argument should already be tracked");
860861
if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[0])) {
861862
for (auto transferredOperand : transferredOperandSet->data()) {
862-
handleLocalUseAfterTransfer(op, op.getOpArgs()[0],
863-
transferredOperand);
863+
handleLocalUseAfterTransferHelper(op, op.getOpArgs()[0],
864+
transferredOperand);
864865
}
865866
}
866867
return;
@@ -873,6 +874,33 @@ struct PartitionOpEvaluator {
873874
for (auto &o : ops)
874875
apply(o);
875876
}
877+
878+
private:
879+
// Private helper that squelches the error if our transfer instruction and our
880+
// use have the same isolation.
881+
void
882+
handleLocalUseAfterTransferHelper(const PartitionOp &op, Element elt,
883+
TransferringOperand *transferringOp) const {
884+
if (auto isolationInfo = SILIsolationInfo::get(op.getSourceInst())) {
885+
if (isolationInfo.isActorIsolated() &&
886+
isolationInfo == SILIsolationInfo::get(transferringOp->getUser()))
887+
return;
888+
}
889+
890+
// If our instruction does not have any isolation info associated with it,
891+
// it must be nonisolated. See if our function has a matching isolation to
892+
// our transferring operand. If so, we can squelch this.
893+
if (auto functionIsolation =
894+
transferringOp->getUser()->getFunction()->getActorIsolation()) {
895+
if (functionIsolation->isActorIsolated() &&
896+
SILIsolationInfo::getActorIsolated(*functionIsolation) ==
897+
SILIsolationInfo::get(transferringOp->getUser()))
898+
return;
899+
}
900+
901+
// Ok, we actually need to emit a call to the callback.
902+
return handleLocalUseAfterTransfer(op, elt, transferringOp);
903+
}
876904
};
877905

878906
/// A base implementation that can be used to default initialize CRTP

‎test/Concurrency/transfernonsendable.swift‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
////////////////////////
1616

1717
/// Classes are always non-sendable, so this is non-sendable
18-
class NonSendableKlass { // expected-complete-note 36{{}}
18+
class NonSendableKlass { // expected-complete-note 38{{}}
1919
// expected-typechecker-only-note @-1 4{{}}
2020
// expected-tns-note @-2 2{{}}
2121
var field: NonSendableKlass? = nil
@@ -129,16 +129,44 @@ func closureInOut(_ a: Actor) async {
129129
// expected-tns-warning @-2 {{transferring 'ns0' may cause a race}}
130130
// expected-tns-note @-3 {{transferring disconnected 'ns0' to actor-isolated callee could cause races in between callee actor-isolated and local nonisolated uses}}
131131

132+
if await booleanFlag {
133+
// This is not an actual use since we are passing values to the same
134+
// isolation domain.
135+
await a.useKlass(ns1)
136+
// expected-complete-warning @-1 {{passing argument of non-sendable type 'NonSendableKlass'}}
137+
} else {
138+
closure() // expected-tns-note {{use here could race}}
139+
}
140+
}
141+
142+
func closureInOutDifferentActor(_ a: Actor, _ a2: Actor) async {
143+
var contents = NonSendableKlass()
144+
let ns0 = NonSendableKlass()
145+
let ns1 = NonSendableKlass()
146+
147+
contents = ns0
148+
contents = ns1
149+
150+
var closure = {}
151+
closure = { useInOut(&contents) }
152+
153+
await a.useKlass(ns0)
154+
// expected-complete-warning @-1 {{passing argument of non-sendable type 'NonSendableKlass'}}
155+
// expected-tns-warning @-2 {{transferring 'ns0' may cause a race}}
156+
// expected-tns-note @-3 {{transferring disconnected 'ns0' to actor-isolated callee could cause races in between callee actor-isolated and local nonisolated uses}}
157+
132158
// We only emit a warning on the first use we see, so make sure we do both
133-
// klass and the closure.
159+
// the print and the closure.
134160
if await booleanFlag {
135-
await a.useKlass(ns1) // expected-tns-note {{use here could race}}
161+
// This is an actual use since a2 is a different actor from a1
162+
await a2.useKlass(ns1)
136163
// expected-complete-warning @-1 {{passing argument of non-sendable type 'NonSendableKlass'}}
137164
} else {
138165
closure() // expected-tns-note {{use here could race}}
139166
}
140167
}
141168

169+
142170
func closureInOut2(_ a: Actor) async {
143171
var contents = NonSendableKlass()
144172
let ns0 = NonSendableKlass()
@@ -1182,9 +1210,11 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive4() async {
11821210
// good... that is QoI though.
11831211
await transferToMain(test) // expected-tns-warning {{transferring 'test' may cause a race}}
11841212
// expected-tns-note @-1 {{transferring disconnected 'test' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
1185-
// expected-tns-note @-2 {{use here could race}}
1186-
// expected-complete-warning @-3 {{passing argument of non-sendable type 'StructFieldTests' into main actor-isolated context may introduce data races}}
1187-
test = StructFieldTests()
1213+
// expected-complete-warning @-2 {{passing argument of non-sendable type 'StructFieldTests' into main actor-isolated context may introduce data races}}
1214+
1215+
// This is treated as a use since test is in box form and is mutable. So we
1216+
// treat assignment as a merge.
1217+
test = StructFieldTests() // expected-tns-note {{use here could race}}
11881218
cls = {
11891219
useInOut(&test.varSendableNonTrivial)
11901220
}
@@ -1215,7 +1245,7 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive5() async {
12151245
}
12161246

12171247
test.varSendableNonTrivial = SendableKlass()
1218-
useValue(test)
1248+
useValue(test) // expected-tns-note {{use here could race}}
12191249
}
12201250

12211251
func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive6() async {
@@ -1387,7 +1417,7 @@ func controlFlowTest2() async {
13871417
x = NonSendableKlass()
13881418
}
13891419

1390-
useValue(x)
1420+
useValue(x) // expected-tns-note {{use here could race}}
13911421
}
13921422

13931423
////////////////////////

‎test/Concurrency/transfernonsendable_global_actor.swift‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,3 +209,15 @@ struct Clock {
209209

210210
useValue(ns)
211211
}
212+
213+
@MainActor func testGlobalAndDisconnected() {
214+
let ns = NonSendableKlass()
215+
216+
let _ = { @MainActor in
217+
print(ns)
218+
}
219+
220+
// Since useValue is running in an actor isolated context, it is ok to use the
221+
// transferred value 'ns' here.
222+
useValue(ns)
223+
}

‎test/Concurrency/transfernonsendable_global_actor_transferring.swift‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,8 @@ func useValue<T>(_ t: T) {}
2727

2828
// Will be resolved once @MainActor is @Sendable
2929
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}}
30+
print(ns)
3231
}
3332

34-
useValue(ns) // expected-note {{use here could race}}
33+
useValue(ns)
3534
}

‎test/Concurrency/transfernonsendable_instruction_matching.sil‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -645,12 +645,14 @@ bb0:
645645

646646
fix_lifetime %value : $SendableKlass
647647

648+
// This is not considered a bad use of the raw pointer since this is
649+
// transferring to the same isolation domain.
648650
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> ()
649651
// expected-warning @-1 {{transferring value of non-Sendable type 'Builtin.RawPointer' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
650-
// expected-note @-2 {{use here could race}}
651652

652653
fix_lifetime %rawPointer : $Builtin.RawPointer
653654
// expected-note @-1 {{use here could race}}
655+
// expected-note @-2 {{use here could race}}
654656

655657
destroy_value %value : $SendableKlass
656658
%9999 = tuple ()
@@ -723,10 +725,10 @@ bb0:
723725

724726
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> ()
725727
// expected-warning @-1 {{transferring value of non-Sendable type 'Builtin.RawPointer' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
726-
// expected-note @-2 {{use here could race}}
727728

728729
fix_lifetime %rawPointer : $Builtin.RawPointer
729730
// expected-note @-1 {{use here could race}}
731+
// expected-note @-2 {{use here could race}}
730732

731733
destroy_value %value : $SendableKlass
732734
%9999 = tuple ()
@@ -802,10 +804,10 @@ bb0:
802804

803805
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> ()
804806
// expected-warning @-1 {{transferring value of non-Sendable type 'Builtin.RawPointer' from nonisolated context to global actor '<null>'-isolated context; later accesses could race}}
805-
// expected-note @-2 {{use here could race}}
806807

807808
fix_lifetime %rawPointer : $Builtin.RawPointer
808809
// expected-note @-1 {{use here could race}}
810+
// expected-note @-2 {{use here could race}}
809811

810812
end_borrow %valueB : $SendableKlass
811813
destroy_value %value : $SendableKlass
@@ -1238,7 +1240,8 @@ bb0:
12381240
unchecked_ref_cast_addr NonSendableKlass in %a : $*NonSendableKlass to NonSendableKlass in %b : $*NonSendableKlass // expected-note {{use here could race}}
12391241

12401242
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context}}
1241-
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
1243+
%use = function_ref @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
1244+
apply %use<NonSendableKlass>(%b) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
12421245

12431246
destroy_addr %b : $*NonSendableKlass
12441247
dealloc_stack %b : $*NonSendableKlass
@@ -1264,7 +1267,8 @@ bb0:
12641267
unconditional_checked_cast_addr NonSendableKlass in %a : $*NonSendableKlass to NonSendableKlass in %b : $*NonSendableKlass // expected-note {{use here could race}}
12651268

12661269
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context}}
1267-
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
1270+
%use = function_ref @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
1271+
apply %use<NonSendableKlass>(%b) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
12681272

12691273
destroy_addr %b : $*NonSendableKlass
12701274
dealloc_stack %b : $*NonSendableKlass
@@ -1290,7 +1294,8 @@ bb0:
12901294
destroy_value %aValue : $NonSendableKlass
12911295

12921296
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<@sil_unowned NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor '<null>'-isolated context}}
1293-
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f<@sil_unowned NonSendableKlass>(%b) : $@convention(thin) @async <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
1297+
%use = function_ref @useIndirect : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> ()
1298+
apply %use<@sil_unowned NonSendableKlass>(%b) : $@convention(thin) <τ_0_0> (@in_guaranteed τ_0_0) -> () // expected-note {{use here could race}}
12941299

12951300
destroy_addr %b : $*@sil_unowned NonSendableKlass
12961301
destroy_addr %a : $*NonSendableKlass

0 commit comments

Comments
 (0)