Skip to content

Commit 62c6de7

Browse files
committed
[region-isolation] Reuse ActorInstance::lookThroughInsts when computing the actor instance for SILIsolationInfo purposes.
We are already using this routine in other parts of TransferNonSendable to ensure that we look through common insts that SILGen inserts that do not change the actual underlying actor instance that we are using. In this case, I added support for casts, optional formation, optional extraction, existential ref initialization. As an example of where this came up is the following test case where we fail to look through an init_existential_ref. ```swift public actor MyActor { private var intDict: [Int: Int] = [:] public func test() async { await withTaskGroup(of: Void.self) { taskGroup in for (_, _) in intDict {} await taskGroup.waitForAll() // Isolation merge failure happens here } } } ``` I also added the ability to at the SIL level actual test out this merge condition using the analysis test runner. I used this to validate that this functionality works as expected in a precise way. rdar://130113744
1 parent 6129f4e commit 62c6de7

File tree

4 files changed

+203
-39
lines changed

4 files changed

+203
-39
lines changed

include/swift/SILOptimizer/Utils/SILIsolationInfo.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,10 @@ class SILDynamicMergedIsolationInfo {
485485
SWIFT_DEBUG_DUMPER(dumpForDiagnostics()) {
486486
innerInfo.dumpForDiagnostics();
487487
}
488+
489+
void printForOneLineLogging(llvm::raw_ostream &os) const {
490+
innerInfo.printForOneLineLogging(os);
491+
}
488492
};
489493

490494
} // namespace swift

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 84 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -364,49 +364,47 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
364364
}
365365

366366
if (auto *isolatedOp = fas.getIsolatedArgumentOperandOrNullPtr()) {
367-
// First see if we have an enum inst.
368-
if (auto *ei = dyn_cast<EnumInst>(isolatedOp->get())) {
369-
if (ei->getElement()->getParentEnum()->isOptionalDecl()) {
370-
// Pattern match from global actors being passed as isolated
371-
// parameters. This gives us better type information. If we can
372-
// pattern match... we should!
373-
if (ei->hasOperand()) {
374-
if (auto *ieri =
375-
dyn_cast<InitExistentialRefInst>(ei->getOperand())) {
376-
CanType selfASTType = ieri->getFormalConcreteType();
377-
378-
if (auto *nomDecl = selfASTType->getAnyActor()) {
379-
// The SILValue() parameter doesn't matter until we have
380-
// isolation history.
381-
if (nomDecl->isGlobalActor())
382-
return SILIsolationInfo::getGlobalActorIsolated(SILValue(),
383-
nomDecl);
384-
}
385-
}
386-
} else {
387-
// In this case, we have a .none so we are attempting to use the
388-
// global queue. In such a case, we need to not use the enum as our
389-
// value and instead need to grab the isolation of our apply.
390-
if (auto isolationInfo = get(fas.getCallee())) {
391-
return isolationInfo;
392-
}
393-
}
367+
// First look through ActorInstance agnostic values so we can find the
368+
// type of the actual underlying actor (e.x.: copy_value,
369+
// init_existential_ref, begin_borrow, etc).
370+
auto actualIsolatedValue =
371+
ActorInstance::lookThroughInsts(isolatedOp->get());
372+
373+
// First see if we have a .none enum inst. In such a case, we are actually
374+
// on the nonisolated global queue.
375+
if (auto *ei = dyn_cast<EnumInst>(actualIsolatedValue)) {
376+
if (ei->getElement()->getParentEnum()->isOptionalDecl() &&
377+
!ei->hasOperand()) {
378+
// In this case, we have a .none so we are attempting to use the
379+
// global queue. This means that the isolation effect of the
380+
// function is disconnected since we are treating the function as
381+
// nonisolated.
382+
return SILIsolationInfo::getDisconnected(false);
394383
}
395384
}
396385

397-
// If we did not find an AST type, just see if we can find a value by
398-
// looking through all optional types. This is conservatively correct.
399-
CanType selfASTType = isolatedOp->get()->getType().getASTType();
386+
// Then using that value, grab the AST type from the actual isolated
387+
// value.
388+
CanType selfASTType = actualIsolatedValue->getType().getASTType();
389+
390+
// Then look through optional types since in cases like where we have a
391+
// function argument that is an Optional actor... like an optional actor
392+
// returned from a function, we still need to be able to lookup the actor
393+
// as being the underlying type.
400394
selfASTType =
401395
selfASTType->lookThroughAllOptionalTypes()->getCanonicalType();
402-
403396
if (auto *nomDecl = selfASTType->getAnyActor()) {
397+
// Then see if we have a global actor. This pattern matches the output
398+
// for doing things like GlobalActor.shared.
399+
if (nomDecl->isGlobalActor()) {
400+
return SILIsolationInfo::getGlobalActorIsolated(SILValue(), nomDecl);
401+
}
402+
404403
// TODO: We really should be doing this based off of an Operand. Then
405404
// we would get the SILValue() for the first element. Today this can
406405
// only mess up isolation history.
407-
408406
return SILIsolationInfo::getActorInstanceIsolated(
409-
SILValue(), isolatedOp->get(), nomDecl);
407+
SILValue(), actualIsolatedValue, nomDecl);
410408
}
411409
}
412410

@@ -1147,11 +1145,33 @@ SILValue ActorInstance::lookThroughInsts(SILValue value) {
11471145
isa<MoveValueInst>(svi) || isa<ExplicitCopyValueInst>(svi) ||
11481146
isa<BeginBorrowInst>(svi) ||
11491147
isa<CopyableToMoveOnlyWrapperValueInst>(svi) ||
1150-
isa<MoveOnlyWrapperToCopyableValueInst>(svi)) {
1148+
isa<MoveOnlyWrapperToCopyableValueInst>(svi) ||
1149+
isa<InitExistentialRefInst>(svi) || isa<UncheckedRefCastInst>(svi) ||
1150+
isa<UnconditionalCheckedCastInst>(svi)) {
11511151
value = lookThroughInsts(svi->getOperand(0));
11521152
continue;
11531153
}
11541154

1155+
// Look Through extracting from optionals.
1156+
if (auto *uedi = dyn_cast<UncheckedEnumDataInst>(svi)) {
1157+
if (uedi->getEnumDecl() ==
1158+
uedi->getFunction()->getASTContext().getOptionalDecl()) {
1159+
value = lookThroughInsts(uedi->getOperand());
1160+
continue;
1161+
}
1162+
}
1163+
1164+
// Look Through wrapping in an enum.
1165+
if (auto *ei = dyn_cast<EnumInst>(svi)) {
1166+
if (ei->hasOperand()) {
1167+
if (ei->getElement()->getParentEnum() ==
1168+
ei->getFunction()->getASTContext().getOptionalDecl()) {
1169+
value = lookThroughInsts(ei->getOperand());
1170+
continue;
1171+
}
1172+
}
1173+
}
1174+
11551175
break;
11561176
}
11571177

@@ -1213,7 +1233,7 @@ SILDynamicMergedIsolationInfo::merge(SILIsolationInfo other) const {
12131233
namespace swift::test {
12141234

12151235
// Arguments:
1216-
// - SILValue: value to emit a name for.
1236+
// - SILValue: value to look up isolation for.
12171237
// Dumps:
12181238
// - The inferred isolation.
12191239
static FunctionTest
@@ -1229,4 +1249,33 @@ static FunctionTest
12291249
llvm::outs() << "\n";
12301250
});
12311251

1252+
// Arguments:
1253+
// - SILValue: first value to merge
1254+
// - SILValue: second value to merge
1255+
// Dumps:
1256+
// - The merged isolation.
1257+
static FunctionTest IsolationMergeTest(
1258+
"sil-isolation-info-merged-inference",
1259+
[](auto &function, auto &arguments, auto &test) {
1260+
auto firstValue = arguments.takeValue();
1261+
auto secondValue = arguments.takeValue();
1262+
SILIsolationInfo firstValueInfo = SILIsolationInfo::get(firstValue);
1263+
SILIsolationInfo secondValueInfo = SILIsolationInfo::get(secondValue);
1264+
std::optional<SILDynamicMergedIsolationInfo> mergedInfo(firstValueInfo);
1265+
mergedInfo = mergedInfo->merge(secondValueInfo);
1266+
llvm::outs() << "First Value: " << *firstValue;
1267+
llvm::outs() << "First Isolation: ";
1268+
firstValueInfo.printForOneLineLogging(llvm::outs());
1269+
llvm::outs() << "\nSecond Value: " << *secondValue;
1270+
llvm::outs() << "Second Isolation: ";
1271+
secondValueInfo.printForOneLineLogging(llvm::outs());
1272+
llvm::outs() << "\nMerged Isolation: ";
1273+
if (mergedInfo) {
1274+
mergedInfo->printForOneLineLogging(llvm::outs());
1275+
} else {
1276+
llvm::outs() << "Merge failure!";
1277+
}
1278+
llvm::outs() << "\n";
1279+
});
1280+
12321281
} // namespace swift::test

test/Concurrency/isolated_parameters.swift

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,9 @@ nonisolated func callFromNonisolated(ns: NotSendable) async {
439439
let myActor = A()
440440

441441
await optionalIsolated(ns, to: myActor)
442-
// expected-complete-warning@-1 {{passing argument of non-sendable type 'NotSendable' into actor-isolated context may introduce data races}}
443-
// expected-tns-warning @-2 {{pattern that the region based isolation checker does not understand how to check. Please file a bug}}
442+
// expected-complete-warning @-1 {{passing argument of non-sendable type 'NotSendable' into actor-isolated context may introduce data races}}
443+
// expected-tns-warning @-2 {{sending 'ns' risks causing data races}}
444+
// expected-tns-note @-3 {{sending task-isolated 'ns' to actor-isolated global function 'optionalIsolated(_:to:)' risks causing data races between actor-isolated and task-isolated uses}}
444445

445446
#if ALLOW_TYPECHECKER_ERRORS
446447
optionalIsolatedSync(ns, to: myActor)
@@ -456,13 +457,14 @@ nonisolated func callFromNonisolated(ns: NotSendable) async {
456457
// expected-tns-warning @-2 {{sending 'ns' risks causing data races}}
457458
// expected-tns-note @-3 {{sending main actor-isolated 'ns' to nonisolated global function 'optionalIsolated(_:to:)' risks causing data races between nonisolated and main actor-isolated uses}}
458459

459-
optionalIsolatedSync(ns, to: nil) // expected-tns-warning {{pattern that the region based isolation checker does not understand how to check. Please file a bug}}
460+
optionalIsolatedSync(ns, to: nil)
460461

461462
let myActor = A()
462463

463464
await optionalIsolated(ns, to: myActor)
464465
// expected-complete-warning@-1 {{passing argument of non-sendable type 'NotSendable' into actor-isolated context may introduce data races}}
465-
// expected-tns-warning @-2 {{pattern that the region based isolation checker does not understand how to check. Please file a bug}}
466+
// expected-tns-warning @-2 {{sending 'ns' risks causing data races}}
467+
// expected-tns-note @-3 {{sending main actor-isolated 'ns' to actor-isolated global function 'optionalIsolated(_:to:)' risks causing data races between actor-isolated and main actor-isolated uses}}
466468

467469
#if ALLOW_TYPECHECKER_ERRORS
468470
optionalIsolatedSync(ns, to: myActor)
@@ -567,3 +569,14 @@ public func useDefaultIsolationWithoutIsolatedParam(
567569
useDefaultIsolation()
568570
useDefaultIsolationWithoutIsolatedParam()
569571
}
572+
573+
public actor MyActorIsolatedParameterMerge {
574+
private var inProgressIndexTasks: [Int: Int] = [:]
575+
576+
public func test() async {
577+
await withTaskGroup(of: Void.self) { taskGroup in
578+
for (_, _) in inProgressIndexTasks {}
579+
await taskGroup.waitForAll()
580+
}
581+
}
582+
}

test/Concurrency/silisolationinfo_inference.sil

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ class NonSendableKlass {
2323

2424
actor MyActor {
2525
var ns: NonSendableKlass
26+
27+
func doSomething() async -> NonSendableKlass
2628
}
2729

2830
sil @transferNonSendableKlass : $@convention(thin) @async (@guaranteed NonSendableKlass) -> ()
@@ -33,6 +35,7 @@ sil @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @o
3335
sil @useUnmanagedNonSendableKlass : $@convention(thin) (@guaranteed @sil_unmanaged NonSendableKlass) -> ()
3436

3537
sil @useActor : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
38+
sil @useOptionalActor : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
3639

3740
///////////////////////
3841
// MARK: Basic Tests //
@@ -721,3 +724,98 @@ bb0(%0 : @owned $MyActor):
721724
destroy_value %1b : $MyActor
722725
return %3 : $NonSendableKlass
723726
}
727+
728+
// CHECK-LABEL: begin running test 1 of 1 on isolated_variable_test_1: sil-isolation-info-inference with: @trace[0]
729+
// CHECK-NEXT: Input Value: %2 = apply %1(%0) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
730+
// CHECK-NEXT: Isolation: disconnected
731+
// CHECK-NEXT: end running test 1 of 1 on isolated_variable_test_1: sil-isolation-info-inference with: @trace[0]
732+
sil [ossa] @isolated_variable_test_1 : $@convention(thin) @async () -> () {
733+
bb0:
734+
specify_test "sil-isolation-info-inference @trace[0]"
735+
%0 = enum $Optional<MyActor>, #Optional.none
736+
%1 = function_ref @useOptionalActor : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
737+
%2 = apply %1(%0) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
738+
//debug_value [trace] %1 : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
739+
debug_value [trace] %2 : $NonSendableKlass
740+
destroy_value %2 : $NonSendableKlass
741+
%9999 = tuple ()
742+
return %9999 : $()
743+
}
744+
745+
sil @$s5infer7MyActorC11doSomethingAA16NonSendableKlassCyYaF : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
746+
747+
// CHECK-LABEL: begin running test 1 of 1 on isolated_variable_test_2:
748+
// CHECK-NEXT: First Value: %3 = apply %2(%0) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
749+
// CHECK-NEXT: First Isolation: 'self'-isolated
750+
// CHECK-NEXT: Second Value: %6 = apply %4(%5) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
751+
// CHECK-NEXT: Second Isolation: 'self'-isolated
752+
// CHECK-NEXT: Merged Isolation: 'self'-isolated
753+
// CHECK-NEXT: end running test 1 of 1 on isolated_variable_test_2: sil-isolation-info-merged-inference with: @trace[0], @trace[1]
754+
sil [ossa] @isolated_variable_test_2 : $@convention(thin) @async (@guaranteed Optional<MyActor>) -> () {
755+
bb0(%0 : @guaranteed $Optional<MyActor>):
756+
debug_value %0 : $Optional<MyActor>, let, name "self"
757+
specify_test "sil-isolation-info-merged-inference @trace[0] @trace[1]"
758+
%1 = function_ref @useOptionalActor : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
759+
%2 = apply %1(%0) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
760+
%3 = function_ref @$s5infer7MyActorC11doSomethingAA16NonSendableKlassCyYaF : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
761+
%0a = unchecked_enum_data %0 : $Optional<MyActor>, #Optional.some!enumelt
762+
%4 = apply %3(%0a) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
763+
debug_value [trace] %2 : $NonSendableKlass
764+
debug_value [trace] %4 : $NonSendableKlass
765+
destroy_value %2 : $NonSendableKlass
766+
destroy_value %4 : $NonSendableKlass
767+
%9999 = tuple ()
768+
return %9999 : $()
769+
}
770+
771+
// CHECK-LABEL: begin running test 1 of 1 on isolated_variable_test_3:
772+
// CHECK-NEXT: First Value: %4 = apply %2(%3) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
773+
// CHECK-NEXT: First Isolation: 'self'-isolated
774+
// CHECK-NEXT: Second Value: %6 = apply %5(%0) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
775+
// CHECK-NEXT: Second Isolation: 'self'-isolated
776+
// CHECK-NEXT: Merged Isolation: 'self'-isolated
777+
// CHECK-NEXT: end running test 1 of 1 on isolated_variable_test_3: sil-isolation-info-merged-inference with: @trace[0], @trace[1]
778+
sil [ossa] @isolated_variable_test_3 : $@convention(thin) @async (@guaranteed MyActor) -> () {
779+
bb0(%0 : @guaranteed $MyActor):
780+
debug_value %0 : $MyActor, let, name "self"
781+
specify_test "sil-isolation-info-merged-inference @trace[0] @trace[1]"
782+
%1 = function_ref @useOptionalActor : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
783+
%0a = enum $Optional<MyActor>, #Optional.some!enumelt, %0 : $MyActor
784+
%2 = apply %1(%0a) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
785+
%3 = function_ref @$s5infer7MyActorC11doSomethingAA16NonSendableKlassCyYaF : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
786+
%4 = apply %3(%0) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
787+
debug_value [trace] %2 : $NonSendableKlass
788+
debug_value [trace] %4 : $NonSendableKlass
789+
destroy_value %2 : $NonSendableKlass
790+
destroy_value %4 : $NonSendableKlass
791+
%9999 = tuple ()
792+
return %9999 : $()
793+
}
794+
795+
// CHECK-LABEL: begin running test 1 of 1 on isolated_variable_test_4:
796+
// CHECK-NEXT: First Value: %8 = apply %2(%7) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
797+
// CHECK-NEXT: First Isolation: 'self'-isolated
798+
// CHECK-NEXT: Second Value: %10 = apply %9(%0) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
799+
// CHECK-NEXT: Second Isolation: 'self'-isolated
800+
// CHECK-NEXT: Merged Isolation: 'self'-isolated
801+
// CHECK-NEXT: end running test 1 of 1 on isolated_variable_test_4: sil-isolation-info-merged-inference with: @trace[0], @trace[1]
802+
sil [ossa] @isolated_variable_test_4 : $@convention(thin) @async (@guaranteed MyActor) -> () {
803+
bb0(%0 : @guaranteed $MyActor):
804+
debug_value %0 : $MyActor, let, name "self"
805+
specify_test "sil-isolation-info-merged-inference @trace[0] @trace[1]"
806+
%1 = function_ref @useOptionalActor : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
807+
%0a = init_existential_ref %0 : $MyActor : $MyActor, $AnyObject
808+
%0b = unconditional_checked_cast %0a : $AnyObject to MyActor
809+
%0c = unchecked_ref_cast %0b : $MyActor to $AnyObject
810+
%0d = unchecked_ref_cast %0c : $AnyObject to $MyActor
811+
%0e = enum $Optional<MyActor>, #Optional.some!enumelt, %0d : $MyActor
812+
%2 = apply %1(%0e) : $@convention(thin) (@sil_isolated @guaranteed Optional<MyActor>) -> @owned NonSendableKlass
813+
%3 = function_ref @$s5infer7MyActorC11doSomethingAA16NonSendableKlassCyYaF : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
814+
%4 = apply %3(%0) : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> @owned NonSendableKlass
815+
debug_value [trace] %2 : $NonSendableKlass
816+
debug_value [trace] %4 : $NonSendableKlass
817+
destroy_value %2 : $NonSendableKlass
818+
destroy_value %4 : $NonSendableKlass
819+
%9999 = tuple ()
820+
return %9999 : $()
821+
}

0 commit comments

Comments
 (0)