Skip to content

Commit 2ccc251

Browse files
committed
[semantic-arc] In SILGen always assign a copy_value's argument to its result.
This ensures that ownership is properly propagated forward through the use-def graph. This was the work that was stymied by issues relating to SILBuilder performing local ARC dataflow. I ripped out that local dataflow in 6f4e2ab and added a cheap ARC guaranteed dataflow pass that performs the same optimization. Also in the process of doing this work, I found that there were many SILGen tests that were either pattern matching in the wrong functions or had wrong CHECK lines (for instance CHECK_NEXT). I fixed all of these issues and also expanded many of the tests so that they verify ownership. The only work I left for a future PR is that there are certain places in tests where we are using the projection from an original value, instead of a copy. I marked those with a message SEMANTIC ARC TODO so that they are easy to find. rdar://28685236
1 parent 3e5c432 commit 2ccc251

File tree

79 files changed

+2594
-1646
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

79 files changed

+2594
-1646
lines changed

Diff for: lib/SILGen/ManagedValue.cpp

+2-4
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ ManagedValue ManagedValue::copy(SILGenFunction &gen, SILLocation l) {
3333
assert(!lowering.isTrivial() && "trivial value has cleanup?");
3434

3535
if (!lowering.isAddressOnly()) {
36-
lowering.emitCopyValue(gen.B, l, getValue());
37-
return gen.emitManagedRValueWithCleanup(getValue(), lowering);
36+
return gen.emitManagedRetain(l, getValue(), lowering);
3837
}
3938

4039
SILValue buf = gen.emitTemporaryAllocation(l, getType());
@@ -66,8 +65,7 @@ ManagedValue ManagedValue::copyUnmanaged(SILGenFunction &gen, SILLocation loc) {
6665

6766
SILValue result;
6867
if (!lowering.isAddressOnly()) {
69-
lowering.emitCopyValue(gen.B, loc, getValue());
70-
result = getValue();
68+
result = lowering.emitCopyValue(gen.B, loc, getValue());
7169
} else {
7270
result = gen.emitTemporaryAllocation(loc, getType());
7371
gen.B.createCopyAddr(loc, getValue(), result, IsNotTake,IsInitialization);

Diff for: lib/SILGen/SILGenApply.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -2344,7 +2344,7 @@ RValue SILGenFunction::emitApply(
23442344
case ParameterConvention::Direct_Owned:
23452345
// If the callee will consume the 'self' parameter, let's retain it so we
23462346
// can keep it alive.
2347-
B.emitCopyValueOperation(loc, lifetimeExtendedSelf);
2347+
lifetimeExtendedSelf = B.emitCopyValueOperation(loc, lifetimeExtendedSelf);
23482348
break;
23492349
case ParameterConvention::Direct_Guaranteed:
23502350
case ParameterConvention::Direct_Unowned:
@@ -2425,7 +2425,7 @@ RValue SILGenFunction::emitApply(
24252425

24262426
case ResultConvention::Unowned:
24272427
// Unretained. Retain the value.
2428-
resultTL.emitCopyValue(B, loc, result);
2428+
result = resultTL.emitCopyValue(B, loc, result);
24292429
break;
24302430
}
24312431

@@ -5448,7 +5448,7 @@ static SILValue emitDynamicPartialApply(SILGenFunction &gen,
54485448
// Retain 'self' because the partial apply will take ownership.
54495449
// We can't simply forward 'self' because the partial apply is conditional.
54505450
if (!self->getType().isAddress())
5451-
gen.B.emitCopyValueOperation(loc, self);
5451+
self = gen.B.emitCopyValueOperation(loc, self);
54525452

54535453
SILValue result = gen.B.createPartialApply(loc, method, method->getType(), {},
54545454
self, partialApplyTy);

Diff for: lib/SILGen/SILGenBridging.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -883,8 +883,7 @@ static SILValue emitObjCUnconsumedArgument(SILGenFunction &gen,
883883
return tmp;
884884
}
885885

886-
lowering.emitCopyValue(gen.B, loc, arg);
887-
return arg;
886+
return lowering.emitCopyValue(gen.B, loc, arg);
888887
}
889888

890889
/// Bridge argument types and adjust retain count conventions for an ObjC thunk.

Diff for: lib/SILGen/SILGenConstructor.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ void SILGenFunction::emitValueConstructor(ConstructorDecl *ctor) {
279279
B.createLoad(cleanupLoc, selfLV, LoadOwnershipQualifier::Unqualified);
280280

281281
// Emit a retain of the loaded value, since we return it +1.
282-
lowering.emitCopyValue(B, cleanupLoc, selfValue);
282+
selfValue = lowering.emitCopyValue(B, cleanupLoc, selfValue);
283283

284284
// Inject the self value into an optional if the constructor is failable.
285285
if (ctor->getFailability() != OTK_None) {
@@ -675,7 +675,7 @@ void SILGenFunction::emitClassConstructorInitializer(ConstructorDecl *ctor) {
675675
}
676676

677677
// We have to do a retain because we are returning the pointer +1.
678-
B.emitCopyValueOperation(cleanupLoc, selfArg);
678+
selfArg = B.emitCopyValueOperation(cleanupLoc, selfArg);
679679

680680
// Inject the self value into an optional if the constructor is failable.
681681
if (ctor->getFailability() != OTK_None) {

Diff for: lib/SILGen/SILGenConvert.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ ManagedValue SILGenFunction::emitProtocolMetatypeToObject(SILLocation loc,
719719
// reference when we use it to prevent it being released and attempting to
720720
// deallocate itself. It doesn't matter if we ever actually clean up that
721721
// retain though.
722-
B.createCopyValue(loc, value);
722+
value = B.createCopyValue(loc, value);
723723
return ManagedValue::forUnmanaged(value);
724724
}
725725

Diff for: lib/SILGen/SILGenDynamicCast.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ namespace {
234234
SGFContext ctx) {
235235
// Retain the result if this is copy-on-success.
236236
if (!shouldTakeOnSuccess(consumption))
237-
origTargetTL.emitCopyValue(SGF.B, Loc, value);
237+
value = origTargetTL.emitCopyValue(SGF.B, Loc, value);
238238

239239
// Enter a cleanup for the +1 result.
240240
ManagedValue result

Diff for: lib/SILGen/SILGenExpr.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ ManagedValue SILGenFunction::emitManagedRetain(SILLocation loc,
5959
return ManagedValue::forUnmanaged(v);
6060
assert(!lowering.isAddressOnly() && "cannot retain an unloadable type");
6161

62-
lowering.emitCopyValue(B, loc, v);
62+
v = lowering.emitCopyValue(B, loc, v);
6363
return emitManagedRValueWithCleanup(v, lowering);
6464
}
6565

@@ -3234,8 +3234,7 @@ class AutoreleasingWritebackComponent : public LogicalPathComponent {
32343234
auto strongType = SILType::getPrimitiveObjectType(
32353235
unowned->getType().castTo<UnmanagedStorageType>().getReferentType());
32363236
auto owned = gen.B.createUnmanagedToRef(loc, unowned, strongType);
3237-
gen.B.createCopyValue(loc, owned);
3238-
auto ownedMV = gen.emitManagedRValueWithCleanup(owned);
3237+
auto ownedMV = gen.emitManagedRetain(loc, owned);
32393238

32403239
// Reassign the +1 storage with it.
32413240
ownedMV.assignInto(gen, loc, base.getUnmanagedValue());

Diff for: lib/SILGen/SILGenFunction.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ void SILGenFunction::emitCaptures(SILLocation loc,
279279
}
280280

281281
// Just retain a by-val let.
282-
B.emitCopyValueOperation(loc, Val);
282+
Val = B.emitCopyValueOperation(loc, Val);
283283
} else {
284284
// If we have a mutable binding for a 'let', such as 'self' in an
285285
// 'init' method, load it.
@@ -321,8 +321,7 @@ void SILGenFunction::emitCaptures(SILLocation loc,
321321
if (canGuarantee) {
322322
capturedArgs.push_back(ManagedValue::forUnmanaged(vl.box));
323323
} else {
324-
B.createCopyValue(loc, vl.box);
325-
capturedArgs.push_back(emitManagedRValueWithCleanup(vl.box));
324+
capturedArgs.push_back(emitManagedRetain(loc, vl.box));
326325
}
327326
escapesToMark.push_back(vl.value);
328327
} else {

Diff for: lib/SILGen/SILGenLValue.cpp

+3-5
Original file line numberDiff line numberDiff line change
@@ -1171,7 +1171,7 @@ namespace {
11711171
// we've still got a release active.
11721172
SILValue baseValue = (isFinal ? base.forward(gen) : base.getValue());
11731173
if (!isFinal)
1174-
gen.B.createCopyValue(loc, baseValue);
1174+
baseValue = gen.B.createCopyValue(loc, baseValue);
11751175

11761176
gen.B.createStrongUnpin(loc, baseValue, Atomicity::Atomic);
11771177
}
@@ -2162,8 +2162,7 @@ SILValue SILGenFunction::emitConversionToSemanticRValue(SILLocation loc,
21622162
auto result = B.createUnmanagedToRef(loc, src,
21632163
SILType::getPrimitiveObjectType(unmanagedType.getReferentType()));
21642164
// SEMANTIC ARC TODO: Does this need a cleanup?
2165-
B.createCopyValue(loc, result);
2166-
return result;
2165+
return B.createCopyValue(loc, result);
21672166
}
21682167

21692168
llvm_unreachable("unexpected storage type that differs from type-of-rvalue");
@@ -2207,8 +2206,7 @@ static SILValue emitLoadOfSemanticRValue(SILGenFunction &gen,
22072206
auto result = gen.B.createUnmanagedToRef(loc, value,
22082207
SILType::getPrimitiveObjectType(unmanagedType.getReferentType()));
22092208
// SEMANTIC ARC TODO: Does this need a cleanup?
2210-
gen.B.createCopyValue(loc, result);
2211-
return result;
2209+
return gen.B.createCopyValue(loc, result);
22122210
}
22132211

22142212
// NSString * must be bridged to String.

Diff for: lib/SILGen/SILGenPoly.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ static ManagedValue manageParam(SILGenFunction &gen,
738738
// Unowned parameters are only guaranteed at the instant of the call, so we
739739
// must retain them even if we're in a context that can accept a +0 value.
740740
case ParameterConvention::Direct_Unowned:
741-
gen.getTypeLowering(paramValue->getType())
741+
paramValue = gen.getTypeLowering(paramValue->getType())
742742
.emitCopyValue(gen.B, loc, paramValue);
743743
SWIFT_FALLTHROUGH;
744744
case ParameterConvention::Direct_Owned:
@@ -2121,8 +2121,7 @@ void ResultPlanner::execute(ArrayRef<SILValue> innerDirectResults,
21212121
"reabstraction of returns_inner_pointer function");
21222122
SWIFT_FALLTHROUGH;
21232123
case ResultConvention::Unowned:
2124-
resultTL.emitCopyValue(Gen.B, Loc, resultValue);
2125-
return Gen.emitManagedRValueWithCleanup(resultValue, resultTL);
2124+
return Gen.emitManagedRetain(Loc, resultValue, resultTL);
21262125
}
21272126
llvm_unreachable("bad result convention!");
21282127
};

Diff for: test/ClangImporter/optional.swift

+11-6
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,11 @@ class A {
1212
return ""
1313
}
1414
// CHECK-LABEL: sil hidden [thunk] @_TToFC8optional1A3foofT_GSqSS_ : $@convention(objc_method) (A) -> @autoreleased Optional<NSString>
15+
// CHECK: bb0([[SELF:%.*]] : $A):
16+
// CHECK: [[SELF_COPY:%.*]] = copy_value [[SELF]]
1517
// CHECK: [[T0:%.*]] = function_ref @_TFC8optional1A3foofT_GSqSS_
16-
// CHECK-NEXT: [[T1:%.*]] = apply [[T0]](%0)
17-
// CHECK-NEXT: destroy_value
18+
// CHECK-NEXT: [[T1:%.*]] = apply [[T0]]([[SELF_COPY]])
19+
// CHECK-NEXT: destroy_value [[SELF_COPY]]
1820
// CHECK: [[T2:%.*]] = select_enum [[T1]]
1921
// CHECK-NEXT: cond_br [[T2]]
2022
// Something branch: project value, translate, inject into result.
@@ -33,10 +35,13 @@ class A {
3335

3436
@objc func bar(x x : String?) {}
3537
// CHECK-LABEL: sil hidden [thunk] @_TToFC8optional1A3barfT1xGSqSS__T_ : $@convention(objc_method) (Optional<NSString>, A) -> ()
36-
// CHECK: [[T1:%.*]] = select_enum %0
38+
// CHECK: bb0([[ARG:%.*]] : $Optional<NSString>, [[SELF:%.*]] : $A):
39+
// CHECK: [[ARG_COPY:%.*]] = copy_value [[ARG]]
40+
// CHECK: [[SELF_COPY:%.*]] = copy_value [[SELF]]
41+
// CHECK: [[T1:%.*]] = select_enum [[ARG_COPY]]
3742
// CHECK-NEXT: cond_br [[T1]]
3843
// Something branch: project value, translate, inject into result.
39-
// CHECK: [[NSSTR:%.*]] = unchecked_enum_data %0
44+
// CHECK: [[NSSTR:%.*]] = unchecked_enum_data [[ARG_COPY]]
4045
// CHECK: [[T0:%.*]] = function_ref @_TZFE10FoundationSS36_unconditionallyBridgeFromObjectiveCfGSqCSo8NSString_SS
4146
// Make a temporary initialized string that we're going to clobber as part of the conversion process (?).
4247
// CHECK-NEXT: [[NSSTR_BOX:%.*]] = enum $Optional<NSString>, #Optional.some!enumelt.1, [[NSSTR]] : $NSString
@@ -50,8 +55,8 @@ class A {
5055
// Continuation.
5156
// CHECK: bb3([[T0:%.*]] : $Optional<String>):
5257
// CHECK: [[T1:%.*]] = function_ref @_TFC8optional1A3barfT1xGSqSS__T_
53-
// CHECK-NEXT: [[T2:%.*]] = apply [[T1]]([[T0]], %1)
54-
// CHECK-NEXT: destroy_value %1
58+
// CHECK-NEXT: [[T2:%.*]] = apply [[T1]]([[T0]], [[SELF_COPY]])
59+
// CHECK-NEXT: destroy_value [[SELF_COPY]]
5560
// CHECK-NEXT: return [[T2]] : $()
5661
}
5762

Diff for: test/SILGen/address_only_types.swift

+4-2
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ func address_only_return(_ x: Unloadable, y: Int) -> Unloadable {
3434
// CHECK: bb0([[RET:%[0-9]+]] : $*Unloadable, [[XARG:%[0-9]+]] : $*Unloadable, [[YARG:%[0-9]+]] : $Builtin.Int64):
3535
// CHECK-NEXT: debug_value_addr [[XARG]] : $*Unloadable, let, name "x"
3636
// CHECK-NEXT: debug_value [[YARG]] : $Builtin.Int64, let, name "y"
37-
// CHECK-NEXT: copy_addr [take] [[XARG]] to [initialization] [[RET]]
37+
// CHECK-NEXT: copy_addr [[XARG]] to [initialization] [[RET]]
38+
// CHECK-NEXT: destroy_addr [[XARG]]
3839
// CHECK-NEXT: [[VOID:%[0-9]+]] = tuple ()
3940
// CHECK-NEXT: return [[VOID]]
4041
return x
@@ -52,7 +53,8 @@ func address_only_conditional_missing_return(_ x: Unloadable) -> Unloadable {
5253
switch Bool.true_ {
5354
case .true_:
5455
// CHECK: [[TRUE]]:
55-
// CHECK: copy_addr [take] %1 to [initialization] %0 : $*Unloadable
56+
// CHECK: copy_addr %1 to [initialization] %0 : $*Unloadable
57+
// CHECK: destroy_addr %1
5658
// CHECK: return
5759
return x
5860
case .false_:

Diff for: test/SILGen/auto_closures.swift

+12-5
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,12 @@
33
struct Bool {}
44
var false_ = Bool()
55

6-
// CHECK-LABEL: sil hidden @_TF13auto_closures17call_auto_closure
6+
// CHECK-LABEL: sil hidden @_TF13auto_closures17call_auto_closureFKT_VS_4BoolS0_ : $@convention(thin) (@owned @callee_owned () -> Bool) -> Bool
77
func call_auto_closure(_ x: @autoclosure () -> Bool) -> Bool {
8-
// CHECK: [[RET:%.*]] = apply %0()
8+
// CHECK: bb0([[CLOSURE:%.*]] : $@callee_owned () -> Bool):
9+
// CHECK: [[CLOSURE_COPY:%.*]] = copy_value [[CLOSURE]]
10+
// CHECK: [[RET:%.*]] = apply [[CLOSURE_COPY]]()
11+
// CHECK: destroy_value [[CLOSURE]]
912
// CHECK: return [[RET]]
1013
return x()
1114
}
@@ -34,9 +37,13 @@ public class Base {
3437

3538
public class Sub : Base {
3639
// CHECK-LABEL: sil hidden @_TFC13auto_closures3Subg1xVS_4Bool : $@convention(method) (@guaranteed Sub) -> Bool {
37-
// CHECK: [[AUTOCLOSURE:%.*]] = function_ref @_TFFC13auto_closures3Subg1xVS_4Boolu_KT_S1_ : $@convention(thin) (@owned Sub) -> Bool
38-
// CHECK: = partial_apply [[AUTOCLOSURE]](%0)
39-
// CHECK: return {{%.*}} : $Bool
40+
// CHECK: bb0([[SELF:%.*]] : $Sub):
41+
// CHECK: [[AUTOCLOSURE_CONSUMER:%.*]] = function_ref @_TF13auto_closures17call_auto_closureFKT_VS_4BoolS0_ : $@convention(thin)
42+
// CHECK: [[AUTOCLOSURE_FUNC:%.*]] = function_ref @_TFFC13auto_closures3Subg1xVS_4Boolu_KT_S1_ : $@convention(thin) (@owned Sub) -> Bool
43+
// CHECK: [[SELF_COPY:%.*]] = copy_value [[SELF]]
44+
// CHECK: [[AUTOCLOSURE:%.*]] = partial_apply [[AUTOCLOSURE_FUNC]]([[SELF_COPY]])
45+
// CHECK: [[RET:%.*]] = apply [[AUTOCLOSURE_CONSUMER]]([[AUTOCLOSURE]])
46+
// CHECK: return [[RET]] : $Bool
4047
// CHECK: }
4148

4249
// CHECK-LABEL: sil shared [transparent] @_TFFC13auto_closures3Subg1xVS_4Boolu_KT_S1_ : $@convention(thin) (@owned Sub) -> Bool {

Diff for: test/SILGen/boxed_existentials.swift

+26-18
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,12 @@ func test_concrete_erasure(_ x: ClericalError) -> Error {
1818
return x
1919
}
2020
// CHECK-LABEL: sil hidden @_TF18boxed_existentials21test_concrete_erasureFOS_13ClericalErrorPs5Error_
21+
// CHECK: bb0([[ARG:%.*]] : $ClericalError):
2122
// CHECK: [[EXISTENTIAL:%.*]] = alloc_existential_box $Error, $ClericalError
2223
// CHECK: [[ADDR:%.*]] = project_existential_box $ClericalError in [[EXISTENTIAL]] : $Error
23-
// CHECK: store %0 to [init] [[ADDR]] : $*ClericalError
24+
// CHECK: [[ARG_COPY:%.*]] = copy_value [[ARG]]
25+
// CHECK: store [[ARG_COPY]] to [init] [[ADDR]] : $*ClericalError
26+
// CHECK: destroy_value [[ARG]]
2427
// CHECK: return [[EXISTENTIAL]] : $Error
2528

2629
protocol HairType {}
@@ -68,24 +71,29 @@ func test_property_of_lvalue(_ x: Error) -> String {
6871
var x = x
6972
return x._domain
7073
}
71-
// CHECK-LABEL: sil hidden @_TF18boxed_existentials23test_property_of_lvalueFPs5Error_SS
74+
75+
// CHECK-LABEL: sil hidden @_TF18boxed_existentials23test_property_of_lvalueFPs5Error_SS :
76+
// CHECK: bb0([[ARG:%.*]] : $Error):
7277
// CHECK: [[VAR:%.*]] = alloc_box $@box Error
73-
// CHECK-NEXT: [[PVAR:%.*]] = project_box [[VAR]]
74-
// CHECK-NEXT: copy_value %0 : $Error
75-
// CHECK-NEXT: store %0 to [init] [[PVAR]]
76-
// CHECK-NEXT: [[VALUE_BOX:%.*]] = load [[PVAR]]
77-
// CHECK-NEXT: copy_value [[VALUE_BOX]]
78-
// CHECK-NEXT: [[VALUE:%.*]] = open_existential_box [[VALUE_BOX]] : $Error to $*[[VALUE_TYPE:@opened\(.*\) Error]]
79-
// CHECK-NEXT: [[COPY:%.*]] = alloc_stack $[[VALUE_TYPE]]
80-
// CHECK-NEXT: copy_addr [[VALUE]] to [initialization] [[COPY]]
81-
// CHECK-NEXT: [[METHOD:%.*]] = witness_method $[[VALUE_TYPE]], #Error._domain!getter.1
82-
// CHECK-NEXT: [[RESULT:%.*]] = apply [[METHOD]]<[[VALUE_TYPE]]>([[COPY]])
83-
// CHECK-NEXT: destroy_addr [[COPY]]
84-
// CHECK-NEXT: dealloc_stack [[COPY]]
85-
// CHECK-NEXT: destroy_value [[VALUE_BOX]]
86-
// CHECK-NEXT: destroy_value [[VAR]]
87-
// CHECK-NEXT: destroy_value %0
88-
// CHECK-NEXT: return [[RESULT]]
78+
// CHECK: [[PVAR:%.*]] = project_box [[VAR]]
79+
// CHECK: [[ARG_COPY:%.*]] = copy_value [[ARG]] : $Error
80+
// CHECK: store [[ARG_COPY]] to [init] [[PVAR]]
81+
// CHECK: [[VALUE_BOX:%.*]] = load [[PVAR]]
82+
// CHECK: [[VALUE_BOX_COPY:%.*]] = copy_value [[VALUE_BOX]]
83+
// ==> SEMANTIC ARC TODO: The next line should take VALUE_BOX_COPY, not VALUE_BOX
84+
// CHECK: [[VALUE:%.*]] = open_existential_box [[VALUE_BOX]] : $Error to $*[[VALUE_TYPE:@opened\(.*\) Error]]
85+
// CHECK: [[COPY:%.*]] = alloc_stack $[[VALUE_TYPE]]
86+
// CHECK: copy_addr [[VALUE]] to [initialization] [[COPY]]
87+
// CHECK: [[METHOD:%.*]] = witness_method $[[VALUE_TYPE]], #Error._domain!getter.1
88+
// CHECK: [[RESULT:%.*]] = apply [[METHOD]]<[[VALUE_TYPE]]>([[COPY]])
89+
// CHECK: destroy_addr [[COPY]]
90+
// CHECK: dealloc_stack [[COPY]]
91+
// ==> SEMANTIC ARC TODO: The next line should take VALUE_BOX_COPY, not VALUE_BOX.
92+
// CHECK: destroy_value [[VALUE_BOX]]
93+
// CHECK: destroy_value [[VAR]]
94+
// CHECK: destroy_value [[ARG]]
95+
// CHECK: return [[RESULT]]
96+
// CHECK: } // end sil function '_TF18boxed_existentials23test_property_of_lvalueFPs5Error_SS'
8997

9098
extension Error {
9199
final func extensionMethod() { }

0 commit comments

Comments
 (0)