Skip to content

Commit 93b7608

Browse files
swift-ciswift-ci
committed
Merge remote-tracking branch 'origin/master' into master-rebranch
2 parents 8ce3f26 + ab9f665 commit 93b7608

File tree

3 files changed

+149
-14
lines changed

3 files changed

+149
-14
lines changed

‎lib/SILOptimizer/Mandatory/MandatoryInlining.cpp

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,21 @@ static void fixupReferenceCounts(
137137
}
138138

139139
visitedBlocks.clear();
140+
140141
// If we need to insert compensating destroys, do so.
142+
//
143+
// NOTE: We use pai here since in non-ossa code emitCopyValueOperation
144+
// returns the operand of the strong_retain which may have a ValueBase
145+
// that is not in the same block. An example of where this is important is
146+
// if we are performing emitCopyValueOperation in non-ossa code on an
147+
// argument when the partial_apply is not in the entrance block. In truth,
148+
// the linear lifetime checker does not /actually/ care what the value is
149+
// (ignoring diagnostic error msgs that we do not care about here), it
150+
// just cares about the block the value is in. In a forthcoming commit, I
151+
// am going to change this to use a different API on the linear lifetime
152+
// checker that makes this clearer.
141153
auto error =
142-
valueHasLinearLifetime(copy, {applySite}, {}, visitedBlocks,
154+
valueHasLinearLifetime(pai, {applySite}, {}, visitedBlocks,
143155
deadEndBlocks, errorBehavior, &leakingBlocks);
144156
if (error.getFoundLeak()) {
145157
while (!leakingBlocks.empty()) {
@@ -175,11 +187,22 @@ static void fixupReferenceCounts(
175187
// TODO: Do we need to lifetime extend here?
176188
case ParameterConvention::Direct_Unowned: {
177189
v = SILBuilderWithScope(pai).emitCopyValueOperation(loc, v);
178-
179190
visitedBlocks.clear();
191+
180192
// If we need to insert compensating destroys, do so.
193+
//
194+
// NOTE: We use pai here since in non-ossa code emitCopyValueOperation
195+
// returns the operand of the strong_retain which may have a ValueBase
196+
// that is not in the same block. An example of where this is important is
197+
// if we are performing emitCopyValueOperation in non-ossa code on an
198+
// argument when the partial_apply is not in the entrance block. In truth,
199+
// the linear lifetime checker does not /actually/ care what the value is
200+
// (ignoring diagnostic error msgs that we do not care about here), it
201+
// just cares about the block the value is in. In a forthcoming commit, I
202+
// am going to change this to use a different API on the linear lifetime
203+
// checker that makes this clearer.
181204
auto error =
182-
valueHasLinearLifetime(v, {applySite}, {}, visitedBlocks,
205+
valueHasLinearLifetime(pai, {applySite}, {}, visitedBlocks,
183206
deadEndBlocks, errorBehavior, &leakingBlocks);
184207
if (error.getFoundError()) {
185208
while (!leakingBlocks.empty()) {
@@ -203,11 +226,22 @@ static void fixupReferenceCounts(
203226
// apply has another use that would destroy our value first.
204227
case ParameterConvention::Direct_Owned: {
205228
v = SILBuilderWithScope(pai).emitCopyValueOperation(loc, v);
206-
207229
visitedBlocks.clear();
230+
208231
// If we need to insert compensating destroys, do so.
232+
//
233+
// NOTE: We use pai here since in non-ossa code emitCopyValueOperation
234+
// returns the operand of the strong_retain which may have a ValueBase
235+
// that is not in the same block. An example of where this is important is
236+
// if we are performing emitCopyValueOperation in non-ossa code on an
237+
// argument when the partial_apply is not in the entrance block. In truth,
238+
// the linear lifetime checker does not /actually/ care what the value is
239+
// (ignoring diagnostic error msgs that we do not care about here), it
240+
// just cares about the block the value is in. In a forthcoming commit, I
241+
// am going to change this to use a different API on the linear lifetime
242+
// checker that makes this clearer.
209243
auto error =
210-
valueHasLinearLifetime(v, {applySite}, {}, visitedBlocks,
244+
valueHasLinearLifetime(pai, {applySite}, {}, visitedBlocks,
211245
deadEndBlocks, errorBehavior, &leakingBlocks);
212246
if (error.getFoundError()) {
213247
while (!leakingBlocks.empty()) {

‎test/Interpreter/mandatory_inlining.swift

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,60 @@
22

33
// REQUIRES: executable_test
44

5-
func test(reportError: ((String) -> (Void))? = nil) {
6-
let reportError = reportError ?? { error in
7-
print(error)
5+
// This file contains various scenarios that validate that various functional
6+
// behaviors of mandatory inlining work as expected. Please give each test a
7+
// descriptive name and a large comment explaining what you are testing.
8+
9+
import StdlibUnittest
10+
11+
var Tests = TestSuite("MandatoryInlining Functional Tests")
12+
defer { runAllTests() }
13+
14+
func doNotInsertReleaseInLoopIfValueConsumedInLoop(
15+
callBack: ((String) -> (Void))? = nil) {
16+
let callBack = callBack ?? { msg in
17+
print(msg)
818
}
919

1020
for _ in 0..<1 {
11-
reportError("foo bar baz")
21+
callBack("foo bar baz")
1222
}
1323
}
1424

15-
func main() {
16-
test { error in
17-
print(error)
25+
// When using the linear lifetime checker to insert compensating releases, if we
26+
// find a double consume due to a loop, do not insert an apply at that call site
27+
// that is in the loop. There is already a compensating retain in the loop that
28+
// we are ignoring. This test functionally makes sure we no longer do this.
29+
Tests.test("doNotInsertReleaseInLoopIfValueConsumedInLoop") {
30+
doNotInsertReleaseInLoopIfValueConsumedInLoop { msg in
31+
print(msg)
1832
}
1933
}
2034

21-
main()
35+
36+
@propertyWrapper
37+
public struct Published<Value> {
38+
public var wrappedValue: Value
39+
public init(initialValue value: Value) { wrappedValue = value }
40+
}
41+
42+
private class Model {
43+
var selectedContact: Int? {
44+
get { _selectedContact }
45+
set {
46+
if let contact = newValue {
47+
self._selectedContact = contact
48+
}
49+
}
50+
}
51+
52+
@Published private var _selectedContact: Int? = nil
53+
}
54+
55+
Tests.test("testNonOSSAPartialApplyLifetimeExtension") {
56+
// If we mess this up, when we set model.selectedContact to nil, we release
57+
// self incorrectly due to mandatory inlining getting the wrong begin lifetime
58+
// block (which in this case is the entry block).
59+
let model = Model()
60+
model.selectedContact = nil
61+
}

‎test/SILOptimizer/mandatory_inlining.sil

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1320,7 +1320,7 @@ bb5:
13201320
// CHECK-NEXT: strong_release [[ARG]]
13211321
// CHECK-NEXT: tuple
13221322
// CHECK-NEXT: return
1323-
// CHECK-NEXT: } // end sil function 'test_apply_pai_in_diamond'
1323+
// CHECK: } // end sil function 'test_apply_pai_in_diamond'
13241324
sil @test_apply_pai_in_diamond : $@convention(thin) (@guaranteed C) -> () {
13251325
bb0(%0: $C):
13261326
strong_retain %0 : $C
@@ -1340,3 +1340,64 @@ bb3:
13401340
%tuple = tuple()
13411341
return %tuple : $()
13421342
}
1343+
1344+
// CHECK-LABEL: sil @test_apply_pai_in_diamond_non_post_dominate_non_post_dominate : $@convention(thin) (@guaranteed C) -> () {
1345+
// CHECK: bb0([[ARG:%.*]] : $C):
1346+
// CHECK-NEXT: cond_br undef, [[BB_TRAMPOLINE:bb[0-9]+]], [[BB_DIAMOND_BEGIN:bb[0-9]+]]
1347+
//
1348+
// CHECK: [[BB_TRAMPOLINE]]:
1349+
// CHECK-NEXT: br [[BB_END:bb[0-9]+]]
1350+
//
1351+
// CHECK: [[BB_DIAMOND_BEGIN]]:
1352+
// CHECK-NEXT: strong_retain [[ARG]]
1353+
// CHECK-NEXT: strong_retain [[ARG]]
1354+
// CHECK-NEXT: cond_br undef, [[NO_CALL_BB:bb[0-9]+]], [[CALL_BB:bb[0-9]+]]
1355+
//
1356+
// CHECK: [[NO_CALL_BB]]:
1357+
// CHECK-NEXT: strong_release [[ARG]]
1358+
// CHECK-NEXT: br [[MERGE_BB:bb[0-9]+]]
1359+
//
1360+
// CHECK: [[CALL_BB]]:
1361+
// CHECK-NEXT: function_ref use_c
1362+
// CHECK-NEXT: [[FUNC:%.*]] = function_ref @use_c : $@convention(thin)
1363+
// CHECK-NEXT: apply [[FUNC]]
1364+
// CHECK-NEXT: tuple
1365+
// CHECK-NEXT: strong_release [[ARG]]
1366+
// CHECK-NEXT: br [[MERGE_BB]]
1367+
//
1368+
// CHECK: [[MERGE_BB]]:
1369+
// CHECK-NEXT: strong_release [[ARG]]
1370+
// CHECK-NEXT: br [[BB_END]]
1371+
//
1372+
// CHECK: [[BB_END]]:
1373+
// CHECK-NEXT: tuple
1374+
// CHECK-NEXT: return
1375+
// CHECK: } // end sil function 'test_apply_pai_in_diamond_non_post_dominate_non_post_dominate'
1376+
sil @test_apply_pai_in_diamond_non_post_dominate_non_post_dominate : $@convention(thin) (@guaranteed C) -> () {
1377+
bb0(%0: $C):
1378+
cond_br undef, bb1, bb2
1379+
1380+
bb1:
1381+
br bb6
1382+
1383+
bb2:
1384+
strong_retain %0 : $C
1385+
%closure_fun = function_ref @guaranteed_closure_func : $@convention(thin) (@guaranteed C) -> ()
1386+
%closure = partial_apply [callee_guaranteed] %closure_fun(%0) : $@convention(thin) (@guaranteed C) -> ()
1387+
cond_br undef, bb3, bb4
1388+
1389+
bb3:
1390+
br bb5
1391+
1392+
bb4:
1393+
apply %closure() : $@callee_guaranteed () -> ()
1394+
br bb5
1395+
1396+
bb5:
1397+
strong_release %closure : $@callee_guaranteed () -> ()
1398+
br bb6
1399+
1400+
bb6:
1401+
%tuple = tuple()
1402+
return %tuple : $()
1403+
}

0 commit comments

Comments
 (0)