Skip to content

Commit 0df5174

Browse files
gottesmmgottesmm
authored
Merge pull request #17599 from gottesmm/swift-4.2-branch-rdar41328987
[4.2][ownership] Do not lower copy_unowned_value to strong_retain_unowned.
2 parents 947fd7d + f0ed0ea commit 0df5174

15 files changed

+97
-26
lines changed

‎include/swift/Serialization/ModuleFormat.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ const uint16_t VERSION_MAJOR = 0;
5555
/// describe what change you made. The content of this comment isn't important;
5656
/// it just ensures a conflict if two people change the module format.
5757
/// Don't worry about adhering to the 80-column limit for this line.
58-
const uint16_t VERSION_MINOR = 414; // Last change: track whether xrefs come from Clang
58+
const uint16_t VERSION_MINOR = 415; // Last change: {strong_retain,copy}_unowned
5959

6060
using DeclIDField = BCFixed<31>;
6161

‎lib/IRGen/IRGenSIL.cpp

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -963,9 +963,7 @@ class IRGenSILFunction :
963963
void visitRetainValueInst(RetainValueInst *i);
964964
void visitRetainValueAddrInst(RetainValueAddrInst *i);
965965
void visitCopyValueInst(CopyValueInst *i);
966-
void visitCopyUnownedValueInst(CopyUnownedValueInst *i) {
967-
llvm_unreachable("unimplemented");
968-
}
966+
void visitCopyUnownedValueInst(CopyUnownedValueInst *i);
969967
void visitReleaseValueInst(ReleaseValueInst *i);
970968
void visitReleaseValueAddrInst(ReleaseValueAddrInst *i);
971969
void visitDestroyValueInst(DestroyValueInst *i);
@@ -3844,6 +3842,18 @@ visitStrongRetainUnownedInst(swift::StrongRetainUnownedInst *i) {
38443842
: irgen::Atomicity::NonAtomic);
38453843
}
38463844

3845+
void IRGenSILFunction::visitCopyUnownedValueInst(
3846+
swift::CopyUnownedValueInst *i) {
3847+
Explosion in = getLoweredExplosion(i->getOperand());
3848+
auto &ti = getReferentTypeInfo(*this, i->getOperand()->getType());
3849+
ti.strongRetainUnowned(*this, in, irgen::Atomicity::Atomic);
3850+
// Semantically we are just passing through the input parameter but as a
3851+
// strong reference... at LLVM IR level these type differences don't
3852+
// matter. So just set the lowered explosion appropriately.
3853+
Explosion output = getLoweredExplosion(i->getOperand());
3854+
setLoweredExplosion(i, output);
3855+
}
3856+
38473857
void IRGenSILFunction::visitUnownedRetainInst(swift::UnownedRetainInst *i) {
38483858
Explosion lowered = getLoweredExplosion(i->getOperand());
38493859
auto &ti = getReferentTypeInfo(*this, i->getOperand()->getType());

‎lib/SIL/SILBuilder.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,8 @@ static bool couldReduceStrongRefcount(SILInstruction *Inst) {
264264
isa<RetainValueInst>(Inst) || isa<UnownedRetainInst>(Inst) ||
265265
isa<UnownedReleaseInst>(Inst) || isa<StrongRetainUnownedInst>(Inst) ||
266266
isa<StoreWeakInst>(Inst) || isa<StrongRetainInst>(Inst) ||
267-
isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst))
267+
isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst) ||
268+
isa<CopyUnownedValueInst>(Inst))
268269
return false;
269270

270271
// Assign and copyaddr of trivial types cannot drop refcounts, and 'inits'

‎lib/SIL/SILVerifier.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1866,9 +1866,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
18661866
"Operand of unowned_retain");
18671867
require(unownedType->isLoadable(ResilienceExpansion::Maximal),
18681868
"unowned_retain requires unowned type to be loadable");
1869-
require(F.hasQualifiedOwnership(),
1870-
"copy_unowned_value is only valid in functions with qualified "
1871-
"ownership");
1869+
// *NOTE* We allow copy_unowned_value to be used throughout the entire
1870+
// pipeline even though it is a higher level instruction.
18721871
}
18731872

18741873
void checkDestroyValueInst(DestroyValueInst *I) {

‎lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,6 +1372,7 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
13721372
case SILInstructionKind::DeallocStackInst:
13731373
case SILInstructionKind::StrongRetainInst:
13741374
case SILInstructionKind::StrongRetainUnownedInst:
1375+
case SILInstructionKind::CopyUnownedValueInst:
13751376
case SILInstructionKind::RetainValueInst:
13761377
case SILInstructionKind::UnownedRetainInst:
13771378
case SILInstructionKind::BranchInst:

‎lib/SILOptimizer/Analysis/MemoryBehavior.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ class MemoryBehaviorVisitor
152152
}
153153
REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainInst)
154154
REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainUnownedInst)
155+
REFCOUNTINC_MEMBEHAVIOR_INST(CopyUnownedValueInst)
155156
REFCOUNTINC_MEMBEHAVIOR_INST(UnownedRetainInst)
156157
REFCOUNTINC_MEMBEHAVIOR_INST(RetainValueInst)
157158
#undef REFCOUNTINC_MEMBEHAVIOR_INST

‎lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,7 @@ void FunctionSideEffects::analyzeInstruction(SILInstruction *I) {
481481
return;
482482
case SILInstructionKind::StrongRetainInst:
483483
case SILInstructionKind::StrongRetainUnownedInst:
484+
case SILInstructionKind::CopyUnownedValueInst:
484485
case SILInstructionKind::RetainValueInst:
485486
case SILInstructionKind::UnownedRetainInst:
486487
getEffectsOn(I->getOperand(0))->Retains = true;

‎lib/SILOptimizer/Mandatory/GuaranteedARCOpts.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ static bool couldReduceStrongRefcount(SILInstruction *Inst) {
7575
if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst) ||
7676
isa<RetainValueInst>(Inst) || isa<UnownedRetainInst>(Inst) ||
7777
isa<UnownedReleaseInst>(Inst) || isa<StrongRetainUnownedInst>(Inst) ||
78+
isa<CopyUnownedValueInst>(Inst) ||
7879
isa<StoreWeakInst>(Inst) || isa<StrongRetainInst>(Inst) ||
7980
isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst) ||
8081
isa<BeginAccessInst>(Inst) || isa<EndAccessInst>(Inst) ||

‎lib/SILOptimizer/Transforms/DeadStoreElimination.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ static bool isDeadStoreInertInstruction(SILInstruction *Inst) {
149149
switch (Inst->getKind()) {
150150
case SILInstructionKind::StrongRetainInst:
151151
case SILInstructionKind::StrongRetainUnownedInst:
152+
case SILInstructionKind::CopyUnownedValueInst:
152153
case SILInstructionKind::UnownedRetainInst:
153154
case SILInstructionKind::RetainValueInst:
154155
case SILInstructionKind::DeallocStackInst:

‎lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ struct OwnershipModelEliminatorVisitor
5757
bool visitStoreInst(StoreInst *SI);
5858
bool visitStoreBorrowInst(StoreBorrowInst *SI);
5959
bool visitCopyValueInst(CopyValueInst *CVI);
60-
bool visitCopyUnownedValueInst(CopyUnownedValueInst *CVI);
6160
bool visitDestroyValueInst(DestroyValueInst *DVI);
6261
bool visitLoadBorrowInst(LoadBorrowInst *LBI);
6362
bool visitBeginBorrowInst(BeginBorrowInst *BBI) {
@@ -160,19 +159,6 @@ bool OwnershipModelEliminatorVisitor::visitCopyValueInst(CopyValueInst *CVI) {
160159
return true;
161160
}
162161

163-
bool OwnershipModelEliminatorVisitor::visitCopyUnownedValueInst(
164-
CopyUnownedValueInst *CVI) {
165-
B.createStrongRetainUnowned(CVI->getLoc(), CVI->getOperand(),
166-
B.getDefaultAtomicity());
167-
// Users of copy_value_unowned expect an owned value. So we need to convert
168-
// our unowned value to a ref.
169-
auto *UTRI =
170-
B.createUnownedToRef(CVI->getLoc(), CVI->getOperand(), CVI->getType());
171-
CVI->replaceAllUsesWith(UTRI);
172-
CVI->eraseFromParent();
173-
return true;
174-
}
175-
176162
bool OwnershipModelEliminatorVisitor::visitUnmanagedRetainValueInst(
177163
UnmanagedRetainValueInst *URVI) {
178164
// Now that we have set the unqualified ownership flag, destroy value

‎lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ static bool isRLEInertInstruction(SILInstruction *Inst) {
156156
case SILInstructionKind::IsUniqueInst:
157157
case SILInstructionKind::IsUniqueOrPinnedInst:
158158
case SILInstructionKind::FixLifetimeInst:
159+
case SILInstructionKind::CopyUnownedValueInst:
159160
return true;
160161
default:
161162
return false;

‎test/IRGen/copy_value_destroy_value.sil

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,22 @@ bb0(%0 : $Builtin.Int32):
3636
// CHECK: call %swift.refcounted* @swift_retain(%swift.refcounted* returned [[VAL2]])
3737
// CHECK: call void @swift_release(%swift.refcounted* [[VAL1]])
3838
// CHECK: call void @swift_release(%swift.refcounted* [[VAL2]])
39-
sil @non_trivial : $@convention(thin) (Foo) -> () {
39+
sil @non_trivial : $@convention(thin) (@guaranteed Foo) -> () {
4040
bb0(%0 : $Foo):
4141
%1 = copy_value %0 : $Foo
4242
destroy_value %1 : $Foo
4343
%2 = tuple()
4444
return %2 : $()
4545
}
46+
47+
// CHECK: define{{( protected)?}} swiftcc void @non_trivial_unowned(
48+
// CHECK: call %swift.refcounted* @swift_unownedRetainStrong(%swift.refcounted* returned %0)
49+
// CHECK: call void @swift_release(%swift.refcounted* %0)
50+
sil @non_trivial_unowned : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
51+
bb0(%0 : $Builtin.NativeObject):
52+
%1 = ref_to_unowned %0 : $Builtin.NativeObject to $@sil_unowned Builtin.NativeObject
53+
%2 = copy_unowned_value %1 : $@sil_unowned Builtin.NativeObject
54+
destroy_value %2 : $Builtin.NativeObject
55+
%9999 = tuple()
56+
return %9999 : $()
57+
}

‎test/Interpreter/strong_retain_unowned_mispairing.swift

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// RUN: %target-run-simple-opt-O-swift
2+
// REQUIRES: executable_test
3+
4+
// We were crashing here due to not preserving rc identity.
5+
// rdar://41328987
6+
7+
func takeEscaping(closure: @escaping (String) -> Void) {}
8+
9+
public class Helper {
10+
weak var o: P?
11+
12+
@_optimize(none)
13+
init(o: P) {
14+
self.o = o
15+
}
16+
}
17+
18+
protocol P: class {}
19+
20+
public class Binding: P {
21+
private var helper: Helper?
22+
23+
public init() {
24+
helper = Helper(o: self)
25+
26+
// Listen to model changes
27+
takeEscaping { [unowned self] (value: String) in
28+
self.update()
29+
}
30+
31+
takeEscaping { [unowned self] (value: String) in
32+
self.update()
33+
}
34+
}
35+
36+
func update() {}
37+
}
38+
39+
@_optimize(none)
40+
func testCrash() {
41+
_ = Binding()
42+
}
43+
44+
testCrash()

‎test/SILOptimizer/ownership_model_eliminator.sil

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,11 +114,12 @@ bb0(%0 : @owned $Builtin.NativeObject):
114114
return %9999 : $()
115115
}
116116

117+
// We no longer lower copy_unowned_value. So make sure that we actually don't.
118+
//
117119
// CHECK-LABEL: sil @copy_unowned_value_test : $@convention(thin) (@owned @sil_unowned Builtin.NativeObject) -> () {
118120
// CHECK: bb0([[ARG:%.*]] : $@sil_unowned Builtin.NativeObject):
119-
// CHECK-NEXT: strong_retain_unowned [[ARG]] : $@sil_unowned Builtin.NativeObject
120-
// CHECK-NEXT: [[OWNED_ARG:%.*]] = unowned_to_ref [[ARG]] : $@sil_unowned Builtin.NativeObject to $Builtin.NativeObject
121-
// CHECK-NEXT: strong_release [[OWNED_ARG]] : $Builtin.NativeObject
121+
// CHECK-NEXT: [[STRONG:%.*]] = copy_unowned_value [[ARG]] : $@sil_unowned Builtin.NativeObject
122+
// CHECK-NEXT: strong_release [[STRONG]] : $Builtin.NativeObject
122123
// CHECK-NEXT: unowned_release [[ARG]] : $@sil_unowned Builtin.NativeObject
123124
// CHECK-NEXT: tuple ()
124125
// CHECK-NEXT: return

‎test/lit.cfg

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -985,6 +985,16 @@ if not getattr(config, 'target_run_simple_swift', None):
985985
'%s %s %%s -o %%t/a.out -module-name main && '
986986
'%s %%t/a.out'
987987
% (config.target_build_swift, mcp_opt, config.target_run))
988+
config.target_run_simple_opt_O_swift = (
989+
'%%empty-directory(%%t) && '
990+
'%s %s -O %%s -o %%t/a.out -module-name main && '
991+
'%s %%t/a.out'
992+
% (config.target_build_swift, mcp_opt, config.target_run))
993+
config.target_run_simple_opt_Osize_swift = (
994+
'%%empty-directory(%%t) && '
995+
'%s %s -Osize %%s -o %%t/a.out -module-name main && '
996+
'%s %%t/a.out'
997+
% (config.target_build_swift, mcp_opt, config.target_run))
988998
config.target_run_stdlib_swift = (
989999
'rm -rf %%t && mkdir -p %%t && '
9901000
'%s %s %%s -o %%t/a.out -module-name main '
@@ -1065,6 +1075,8 @@ config.substitutions.append(('%target-swift-frontend', config.target_swift_front
10651075

10661076
config.substitutions.append(('%target-run-simple-swiftgyb', config.target_run_simple_swiftgyb))
10671077
config.substitutions.append(('%target-run-simple-swift', config.target_run_simple_swift))
1078+
config.substitutions.append(('%target-run-simple-opt-O-swift', config.target_run_simple_opt_O_swift))
1079+
config.substitutions.append(('%target-run-simple-opt-Osize-swift', config.target_run_simple_opt_Osize_swift))
10681080
config.substitutions.append(('%target-run-stdlib-swiftgyb', config.target_run_stdlib_swiftgyb))
10691081
config.substitutions.append(('%target-run-stdlib-swift', config.target_run_stdlib_swift))
10701082
config.substitutions.append(('%target-repl-run-simple-swift', subst_target_repl_run_simple_swift))

0 commit comments

Comments
 (0)