Skip to content

Commit c24889c

Browse files
gottesmmgottesmm
authored
Merge pull request #72004 from gottesmm/main-rdar123881277
[region-isolation] Only make partial_apply actor derived if we capture an isolated parameter... not just any actor.
2 parents f1f35bf + f3e8d8b commit c24889c

File tree

11 files changed

+119
-52
lines changed

11 files changed

+119
-52
lines changed

‎include/swift/AST/CaptureInfo.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,12 @@ class CapturedValue {
100100
return Value.getPointer().is<OpaqueValueExpr *>();
101101
}
102102

103+
/// Returns true if this captured value is a local capture.
104+
///
105+
/// NOTE: This implies that the value is not dynamic self metadata, since
106+
/// values with decls are the only values that are able to be local captures.
107+
bool isLocalCapture() const;
108+
103109
CapturedValue mergeFlags(CapturedValue cv) {
104110
assert(Value.getPointer() == cv.Value.getPointer() &&
105111
"merging flags on two different value decls");

‎include/swift/SIL/ApplySite.h

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,16 @@ class ApplySite {
571571
return getApplyOptions().contains(ApplyFlags::DoesNotAwait);
572572
}
573573

574+
/// Return the SILParameterInfo for this operand in the callee function.
575+
SILParameterInfo getArgumentParameterInfo(const Operand &oper) const {
576+
assert(!getArgumentConvention(oper).isIndirectOutParameter() &&
577+
"Can only be applied to non-out parameters");
578+
579+
// The ParameterInfo is going to be the parameter in the caller.
580+
unsigned calleeArgIndex = getCalleeArgIndex(oper);
581+
return getSubstCalleeConv().getParamInfoForSILArg(calleeArgIndex);
582+
}
583+
574584
static ApplySite getFromOpaqueValue(void *p) { return ApplySite(p); }
575585

576586
friend bool operator==(ApplySite lhs, ApplySite rhs) {
@@ -801,15 +811,6 @@ class FullApplySite : public ApplySite {
801811
}
802812
}
803813

804-
SILParameterInfo getArgumentParameterInfo(const Operand &oper) const {
805-
assert(!getArgumentConvention(oper).isIndirectOutParameter() &&
806-
"Can only be applied to non-out parameters");
807-
808-
// The ParameterInfo is going to be the parameter in the caller.
809-
unsigned calleeArgIndex = getCalleeArgIndex(oper);
810-
return getSubstCalleeConv().getParamInfoForSILArg(calleeArgIndex);
811-
}
812-
813814
static FullApplySite getFromOpaqueValue(void *p) { return FullApplySite(p); }
814815

815816
static bool classof(const SILInstruction *inst) {

‎lib/AST/CaptureInfo.cpp

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@
1717

1818
using namespace swift;
1919

20+
//===----------------------------------------------------------------------===//
21+
// MARK: CaptureInfo
22+
//===----------------------------------------------------------------------===//
23+
2024
CaptureInfo::CaptureInfo(ASTContext &ctx, ArrayRef<CapturedValue> captures,
2125
DynamicSelfType *dynamicSelf,
2226
OpaqueValueExpr *opaqueValue,
@@ -56,9 +60,10 @@ CaptureInfo CaptureInfo::empty() {
5660
}
5761

5862
bool CaptureInfo::hasLocalCaptures() const {
59-
for (auto capture : getCaptures())
60-
if (capture.getDecl()->isLocalCapture())
63+
for (auto capture : getCaptures()) {
64+
if (capture.isLocalCapture())
6165
return true;
66+
}
6267
return false;
6368
}
6469

@@ -71,7 +76,7 @@ getLocalCaptures(SmallVectorImpl<CapturedValue> &Result) const {
7176

7277
// Filter out global variables.
7378
for (auto capture : getCaptures()) {
74-
if (!capture.getDecl()->isLocalCapture())
79+
if (!capture.isLocalCapture())
7580
continue;
7681

7782
Result.push_back(capture);
@@ -83,10 +88,10 @@ VarDecl *CaptureInfo::getIsolatedParamCapture() const {
8388
return nullptr;
8489

8590
for (const auto &capture : getCaptures()) {
86-
if (!capture.getDecl()->isLocalCapture())
87-
continue;
88-
89-
if (capture.isDynamicSelfMetadata())
91+
// NOTE: isLocalCapture() returns false if we have dynamic self metadata
92+
// since dynamic self metadata is never an isolated capture. So we can just
93+
// call isLocalCapture without checking for dynamic self metadata.
94+
if (!capture.isLocalCapture())
9095
continue;
9196

9297
// If we captured an isolated parameter, return it.
@@ -134,3 +139,11 @@ void CaptureInfo::print(raw_ostream &OS) const {
134139
OS << ')';
135140
}
136141

142+
//===----------------------------------------------------------------------===//
143+
// MARK: CapturedValue
144+
//===----------------------------------------------------------------------===//
145+
146+
bool CapturedValue::isLocalCapture() const {
147+
auto *decl = Value.getPointer().dyn_cast<ValueDecl *>();
148+
return decl && decl->isLocalCapture();
149+
}

‎lib/SIL/IR/SILFunctionType.cpp

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1917,6 +1917,7 @@ lowerCaptureContextParameters(TypeConverter &TC, SILDeclRef function,
19171917
// signature from the AST for that.
19181918
auto origGenericSig = function.getAnyFunctionRef()->getGenericSignature();
19191919
auto loweredCaptures = TC.getLoweredLocalCaptures(function);
1920+
auto *isolatedParam = loweredCaptures.getIsolatedParamCapture();
19201921

19211922
for (auto capture : loweredCaptures.getCaptures()) {
19221923
if (capture.isDynamicSelfMetadata()) {
@@ -1955,13 +1956,19 @@ lowerCaptureContextParameters(TypeConverter &TC, SILDeclRef function,
19551956
continue;
19561957
}
19571958

1958-
auto *VD = capture.getDecl();
1959-
auto type = VD->getInterfaceType();
1959+
auto *varDecl = capture.getDecl();
1960+
auto type = varDecl->getInterfaceType();
19601961
auto canType = type->getReducedType(origGenericSig);
19611962

1963+
auto options = SILParameterInfo::Options();
1964+
if (isolatedParam == varDecl) {
1965+
options |= SILParameterInfo::Isolated;
1966+
isolatedParam = nullptr;
1967+
}
1968+
19621969
// If we're capturing a parameter pack, wrap it in a tuple.
19631970
if (isa<PackExpansionType>(canType)) {
1964-
assert(!cast<ParamDecl>(VD)->supportsMutation() &&
1971+
assert(!cast<ParamDecl>(varDecl)->supportsMutation() &&
19651972
"Cannot capture a pack as an lvalue");
19661973

19671974
SmallVector<TupleTypeElt, 1> elts;
@@ -1983,7 +1990,7 @@ lowerCaptureContextParameters(TypeConverter &TC, SILDeclRef function,
19831990
} else {
19841991
convention = ParameterConvention::Direct_Guaranteed;
19851992
}
1986-
SILParameterInfo param(loweredTy.getASTType(), convention);
1993+
SILParameterInfo param(loweredTy.getASTType(), convention, options);
19871994
inputs.push_back(param);
19881995
break;
19891996
}
@@ -1995,10 +2002,10 @@ lowerCaptureContextParameters(TypeConverter &TC, SILDeclRef function,
19952002
.getLoweredType();
19962003
// Lvalues are captured as a box that owns the captured value.
19972004
auto boxTy = TC.getInterfaceBoxTypeForCapture(
1998-
VD, minimalLoweredTy.getASTType(),
2005+
varDecl, minimalLoweredTy.getASTType(),
19992006
/*mutable*/ true);
20002007
auto convention = ParameterConvention::Direct_Guaranteed;
2001-
auto param = SILParameterInfo(boxTy, convention);
2008+
auto param = SILParameterInfo(boxTy, convention, options);
20022009
inputs.push_back(param);
20032010
break;
20042011
}
@@ -2009,35 +2016,38 @@ lowerCaptureContextParameters(TypeConverter &TC, SILDeclRef function,
20092016
TypeExpansionContext::minimal())
20102017
.getLoweredType();
20112018
// Lvalues are captured as a box that owns the captured value.
2012-
auto boxTy =
2013-
TC.getInterfaceBoxTypeForCapture(VD, minimalLoweredTy.getASTType(),
2014-
/*mutable*/ false);
2019+
auto boxTy = TC.getInterfaceBoxTypeForCapture(
2020+
varDecl, minimalLoweredTy.getASTType(),
2021+
/*mutable*/ false);
20152022
auto convention = ParameterConvention::Direct_Guaranteed;
2016-
auto param = SILParameterInfo(boxTy, convention);
2023+
auto param = SILParameterInfo(boxTy, convention, options);
20172024
inputs.push_back(param);
20182025
break;
20192026
}
20202027
case CaptureKind::StorageAddress: {
20212028
// Non-escaping lvalues are captured as the address of the value.
20222029
SILType ty = loweredTy.getAddressType();
2023-
auto param =
2024-
SILParameterInfo(ty.getASTType(),
2025-
ParameterConvention::Indirect_InoutAliasable);
2030+
auto param = SILParameterInfo(
2031+
ty.getASTType(), ParameterConvention::Indirect_InoutAliasable,
2032+
options);
20262033
inputs.push_back(param);
20272034
break;
20282035
}
20292036
case CaptureKind::Immutable: {
20302037
// 'let' constants that are address-only are captured as the address of
20312038
// the value and will be consumed by the closure.
20322039
SILType ty = loweredTy.getAddressType();
2033-
auto param =
2034-
SILParameterInfo(ty.getASTType(),
2035-
ParameterConvention::Indirect_In_Guaranteed);
2040+
auto param = SILParameterInfo(ty.getASTType(),
2041+
ParameterConvention::Indirect_In_Guaranteed,
2042+
options);
20362043
inputs.push_back(param);
20372044
break;
20382045
}
20392046
}
20402047
}
2048+
assert(!isolatedParam &&
2049+
"If we had an isolated capture, we should have visited it when "
2050+
"iterating over loweredCaptures.getCaptures().");
20412051
}
20422052

20432053
static AccessorDecl *

‎lib/SILOptimizer/Analysis/RegionAnalysis.cpp

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1693,18 +1693,19 @@ class PartitionOpTranslator {
16931693
}
16941694

16951695
SILMultiAssignOptions options;
1696-
for (auto arg : pai->getOperandValues()) {
1697-
if (auto value = tryToTrackValue(arg)) {
1696+
for (auto &op : pai->getAllOperands()) {
1697+
if (auto value = tryToTrackValue(op.get())) {
16981698
if (value->isActorDerived()) {
16991699
options |= SILMultiAssignFlags::PropagatesActorSelf;
17001700
}
17011701
} else {
1702-
// NOTE: One may think that only sendable things can enter
1703-
// here... but we treat things like function_ref/class_method which
1704-
// are non-Sendable as sendable for our purposes.
1705-
if (arg->getType().isActor()) {
1702+
// We only treat Sendable values as propagating actor self if the
1703+
// partial apply has operand as an sil_isolated parameter.
1704+
ApplySite applySite(pai);
1705+
if (applySite.isArgumentOperand(op) &&
1706+
ApplySite(pai).getArgumentParameterInfo(op).hasOption(
1707+
SILParameterInfo::Isolated))
17061708
options |= SILMultiAssignFlags::PropagatesActorSelf;
1707-
}
17081709
}
17091710
}
17101711

‎test/Concurrency/transfernonsendable.swift

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -195,14 +195,10 @@ func transferNonIsolatedNonAsyncClosureTwice() async {
195195
var actorCaptureClosure = { print(a) }
196196
await transferToMain(actorCaptureClosure) // expected-complete-warning {{passing argument of non-sendable type '() -> ()' into main actor-isolated context may introduce data races}}
197197
// expected-complete-note @-1 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
198-
// expected-tns-warning @-2 {{transferring 'actorCaptureClosure' may cause a race}}
199-
// expected-tns-note @-3 {{transferring nonisolated 'actorCaptureClosure' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
200198

201199
actorCaptureClosure = { print(a) }
202200
await transferToMain(actorCaptureClosure) // expected-complete-warning {{passing argument of non-sendable type '() -> ()' into main actor-isolated context may introduce data races}}
203201
// expected-complete-note @-1 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
204-
// expected-tns-warning @-2 {{transferring 'actorCaptureClosure' may cause a race}}
205-
// expected-tns-note @-3 {{transferring nonisolated 'actorCaptureClosure' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
206202
}
207203

208204
extension Actor {
@@ -403,8 +399,6 @@ func testSimpleLetClosureCaptureActor() async {
403399
let closure = { print(a) }
404400
await transferToMain(closure) // expected-complete-warning {{passing argument of non-sendable type '() -> ()' into main actor-isolated context may introduce data races}}
405401
// expected-complete-note @-1 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
406-
// expected-tns-warning @-2 {{transferring 'closure' may cause a race}}
407-
// expected-tns-note @-3 {{transferring nonisolated 'closure' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
408402
}
409403

410404
#if TYPECHECKER_ONLY
@@ -441,8 +435,6 @@ func testSimpleVarClosureCaptureActor() async {
441435
closure = { print(a) }
442436
await transferToMain(closure) // expected-complete-warning {{passing argument of non-sendable type '() -> ()' into main actor-isolated context may introduce data races}}
443437
// expected-complete-note @-1 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
444-
// expected-tns-warning @-2 {{transferring 'closure' may cause a race}}
445-
// expected-tns-note @-3 {{transferring nonisolated 'closure' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
446438
}
447439

448440
#if TYPECHECKER_ONLY

‎test/SILGen/capture_info_invariants.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// RUN: %target-swift-frontend -o - -emit-silgen %s
2+
3+
// REQUIRES: concurrency
4+
5+
// READ THIS! This test is meant to test invariants around CaptureInfo usage in
6+
// SILGen. Only add examples to this if we hit a crasher in capture info and the
7+
// example makes sure we do not crash again.
8+
9+
class GetIsolatedParamTest {
10+
public static var rootType: Any.Type? { nil }
11+
}
12+
13+
extension GetIsolatedParamTest : CustomDebugStringConvertible {
14+
public var debugDescription: String {
15+
let description = "\\\(String(describing: Self.rootType!))"
16+
let x: Any.Type? = nil
17+
// The error is triggered by the ?? autoclosure.
18+
var valueType: Any.Type? = x ?? Self.rootType
19+
return description
20+
}
21+
}

‎test/SILGen/check_executor.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public actor MyActor {
4747
}
4848
}
4949

50-
// CHECK-CANONICAL-LABEL: sil private [ossa] @$s4test7MyActorC0A13LocalFunctionyyF5localL_SiyF : $@convention(thin) (@guaranteed MyActor) -> Int
50+
// CHECK-CANONICAL-LABEL: sil private [ossa] @$s4test7MyActorC0A13LocalFunctionyyF5localL_SiyF : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> Int
5151
// CHECK-CANONICAL: [[CAPTURE:%.*]] = copy_value %0 : $MyActor
5252
// CHECK-CANONICAL-NEXT: [[BORROWED_CAPTURE:%.*]] = begin_borrow [[CAPTURE]] : $MyActor
5353
// CHECK-CANONICAL-NEXT: [[EXECUTOR:%.*]] = builtin "buildDefaultActorExecutorRef"<MyActor>([[BORROWED_CAPTURE]] : $MyActor) : $Builtin.Executor

‎test/SILGen/hop_to_executor.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ actor MyActor {
6161
// CHECK: } // end sil function '$s4test7MyActorC0A7ClosureSiyYaF'
6262

6363
///// closure #1 in MyActor.testClosure()
64-
// CHECK-LABEL: sil private [ossa] @$s4test7MyActorC0A7ClosureSiyYaFSiyYaXEfU_ : $@convention(thin) @async (@guaranteed MyActor) -> Int {
64+
// CHECK-LABEL: sil private [ossa] @$s4test7MyActorC0A7ClosureSiyYaFSiyYaXEfU_ : $@convention(thin) @async (@sil_isolated @guaranteed MyActor) -> Int {
6565
// CHECK: [[COPIED_SELF:%[0-9]+]] = copy_value %0 : $MyActor
6666
// CHECK: [[BORROWED_SELF:%[0-9]+]] = begin_borrow [[COPIED_SELF]] : $MyActor
6767
// CHECK: hop_to_executor [[BORROWED_SELF]] : $MyActor

‎test/SILGen/isolated_any.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ func testEraseInheritingSyncMainActorClosure() {
221221

222222
// CHECK-LABEL: sil hidden [ossa] @$s4test7MyActorC0a19EraseInheritingSyncC7ClosureyyF
223223
// CHECK: // function_ref closure #1
224-
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a19EraseInheritingSyncC7ClosureyyFyycfU_ : $@convention(thin) (@guaranteed Optional<any Actor>, @guaranteed MyActor) -> ()
224+
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a19EraseInheritingSyncC7ClosureyyFyycfU_ : $@convention(thin) (@guaranteed Optional<any Actor>, @sil_isolated @guaranteed MyActor) -> ()
225225
// CHECK-NEXT: [[CAPTURE:%.*]] = copy_value %0 : $MyActor
226226
// CHECK-NEXT: [[CAPTURE_FOR_ISOLATION:%.*]] = copy_value [[CAPTURE]] : $MyActor
227227
// CHECK-NEXT: [[ISOLATION_OBJECT:%.*]] = init_existential_ref [[CAPTURE_FOR_ISOLATION]] : $MyActor : $MyActor, $any Actor
@@ -332,7 +332,7 @@ func testEraseInheritingAsyncMainActorClosure() {
332332

333333
// CHECK-LABEL: sil hidden [ossa] @$s4test7MyActorC0a20EraseInheritingAsyncC7ClosureyyF
334334
// CHECK: // function_ref closure #1
335-
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a20EraseInheritingAsyncC7ClosureyyFyyYacfU_ : $@convention(thin) @async (@guaranteed Optional<any Actor>, @guaranteed MyActor) -> ()
335+
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a20EraseInheritingAsyncC7ClosureyyFyyYacfU_ : $@convention(thin) @async (@guaranteed Optional<any Actor>, @sil_isolated @guaranteed MyActor) -> ()
336336
// CHECK-NEXT: [[CAPTURE:%.*]] = copy_value %0 : $MyActor
337337
// CHECK-NEXT: [[CAPTURE_FOR_ISOLATION:%.*]] = copy_value [[CAPTURE]] : $MyActor
338338
// CHECK-NEXT: [[ISOLATION_OBJECT:%.*]] = init_existential_ref [[CAPTURE_FOR_ISOLATION]] : $MyActor : $MyActor, $any Actor
@@ -362,7 +362,7 @@ func takeInheritingAsyncIsolatedAny_optionalResult(@_inheritActorContext fn: @es
362362
// CHECK-LABEL: sil hidden [ossa] @$s4test7MyActorC0a20EraseInheritingAsyncC18Closure_noPeepholeyyF
363363
// We emit the closure itself with just the erasure conversion.
364364
// CHECK: // function_ref closure #1
365-
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a20EraseInheritingAsyncC18Closure_noPeepholeyyFSiyYacfU_ : $@convention(thin) @async (@guaranteed Optional<any Actor>, @guaranteed MyActor) -> Int
365+
// CHECK-NEXT: [[CLOSURE_FN:%.*]] = function_ref @$s4test7MyActorC0a20EraseInheritingAsyncC18Closure_noPeepholeyyFSiyYacfU_ : $@convention(thin) @async (@guaranteed Optional<any Actor>, @sil_isolated @guaranteed MyActor) -> Int
366366
// CHECK-NEXT: [[CAPTURE:%.*]] = copy_value %0 : $MyActor
367367
// CHECK-NEXT: [[CAPTURE_FOR_ISOLATION:%.*]] = copy_value [[CAPTURE]] : $MyActor
368368
// CHECK-NEXT: [[ISOLATION_OBJECT:%.*]] = init_existential_ref [[CAPTURE_FOR_ISOLATION]] : $MyActor : $MyActor, $any Actor

‎test/SILGen/isolated_self_capture.swift

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// RUN: %target-swift-frontend -emit-silgen %s -module-name test -disable-availability-checking -sil-verify-all -swift-version 5 | %FileCheck %s
2+
3+
// REQUIRES: concurrency
4+
5+
class NonSendableKlass {}
6+
7+
func useValue<T>(_ t: T) {}
8+
9+
@MainActor func transferToMain<T>(_ t: T) async {}
10+
11+
actor MyActor {
12+
var ns = NonSendableKlass()
13+
14+
// CHECK-LABEL: sil hidden [ossa] @$s4test7MyActorC19passSelfIntoClosureyyYaF : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> () {
15+
// CHECK: bb0([[ARG:%.*]] : @guaranteed $
16+
// CHECK: [[COPY:%.*]] = copy_value [[ARG]]
17+
// CHECK: [[PAI:%.*]] = partial_apply [callee_guaranteed] {{.*}}([[COPY]]) : $@convention(thin) (@sil_isolated @guaranteed MyActor) -> ()
18+
// CHECK: } // end sil function '$s4test7MyActorC19passSelfIntoClosureyyYaF'
19+
func passSelfIntoClosure() async {
20+
let closure = { useValue(self.ns) }
21+
await transferToMain(closure)
22+
}
23+
}

0 commit comments

Comments
 (0)