Skip to content

Commit f7d30d4

Browse files
committed
[move-only] Fix SILOptimizer expansion to preserve deinits.
Many basic SIL passes were silently deleting the struct deinit: - SROA - Mem2Reg - RLE - DSE - FunctionSignatureOpts - LowerAggregates Fixes rdar://109849028 ([move-only] LowerAggregateInstrs eliminates struct deinitialization)
1 parent 62b0899 commit f7d30d4

File tree

4 files changed

+58
-5
lines changed

4 files changed

+58
-5
lines changed

include/swift/SILOptimizer/Utils/InstOptUtils.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -381,8 +381,10 @@ ignore_expect_uses(ValueBase *value) {
381381
/// operations from it. These can be simplified and removed.
382382
bool simplifyUsers(SingleValueInstruction *inst);
383383

384-
/// True if a type can be expanded
385-
/// without a significant increase to code size.
384+
/// True if a type can be expanded without a significant increase to code size.
385+
///
386+
/// False if expanding a type is invalid. For example, expanding a
387+
/// struct-with-deinit drops the deinit.
386388
bool shouldExpand(SILModule &module, SILType ty);
387389

388390
/// Check if the value of value is computed by means of a simple initialization.

lib/SILOptimizer/Transforms/SILLowerAggregateInstrs.cpp

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,15 @@ static llvm::cl::opt<bool> EnableExpandAll("sil-lower-agg-instrs-expand-all",
4949
/// unnecessary IR. At the same time for testing purposes, we want to provide a
5050
/// way even with ownership enabled to expand so we can check correctness.
5151
static bool shouldExpandShim(SILFunction *fn, SILType type) {
52-
return EnableExpandAll ||
53-
(!fn->hasOwnership() && shouldExpand(fn->getModule(), type));
52+
// shouldExpand returns false for struct-with-deinit types, so bypassing it is
53+
// incorrect for move-only types
54+
if (EnableExpandAll) {
55+
assert(!type.isPureMoveOnly()
56+
&& "sil-lower-agg-instrs-expand-all is incompatible with move-only "
57+
"types");
58+
return true;
59+
}
60+
return !fn->hasOwnership() && shouldExpand(fn->getModule(), type);
5461
}
5562

5663
//===----------------------------------------------------------------------===//

lib/SILOptimizer/Utils/InstOptUtils.cpp

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1120,14 +1120,24 @@ bool swift::simplifyUsers(SingleValueInstruction *inst) {
11201120
return changed;
11211121
}
11221122

1123-
/// True if a type can be expanded without a significant increase to code size.
1123+
// True if a type can be expanded without a significant increase to code size.
1124+
//
1125+
// False if expanding a type is invalid. For example, expanding a
1126+
// struct-with-deinit drops the deinit.
11241127
bool swift::shouldExpand(SILModule &module, SILType ty) {
11251128
// FIXME: Expansion
11261129
auto expansion = TypeExpansionContext::minimal();
11271130

11281131
if (module.Types.getTypeLowering(ty, expansion).isAddressOnly()) {
11291132
return false;
11301133
}
1134+
// A move-only-with-deinit type cannot be SROA.
1135+
//
1136+
// TODO: we could loosen this requirement if all paths lead to a drop_deinit.
1137+
if (auto *nominalTy = ty.getNominalOrBoundGenericNominal()) {
1138+
if (nominalTy->getValueTypeDestructor())
1139+
return false;
1140+
}
11311141
if (EnableExpandAll) {
11321142
return true;
11331143
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// RUN: %target-sil-opt -enable-sil-verify-all -lower-aggregate-instrs %s | %FileCheck %s
2+
3+
sil_stage canonical
4+
5+
import Builtin
6+
7+
struct S : ~Copyable {
8+
deinit {}
9+
}
10+
struct S2 : ~Copyable {
11+
var s1 = S()
12+
var s2 = S()
13+
}
14+
15+
// Test that a struct-with-deinit is preserved on both members s1 and s2.
16+
//
17+
// public func testDeinitTwo() {
18+
// var s = S2()
19+
// }
20+
//
21+
// CHECK-LABEL: sil @testDeinitTwo : $@convention(thin) () -> () {
22+
// CHECK: struct_extract [[S2:%.*]] : $S2, #S2.s1
23+
// CHECK: release_value %{{.*}} : $S
24+
// CHECK: struct_extract [[S2]] : $S2, #S2.s2
25+
// CHECK: release_value %{{.*}} : $S
26+
// CHECK-LABEL: } // end sil function 'testDeinitTwo'
27+
sil @testDeinitTwo : $@convention(thin) () -> () {
28+
bb0:
29+
%0 = struct $S ()
30+
%1 = struct $S2 (%0 : $S, %0 : $S)
31+
release_value %1 : $S2
32+
%5 = tuple ()
33+
return %5 : $()
34+
}

0 commit comments

Comments
 (0)