Skip to content

Commit 42c1f9c

Browse files
meg-guptanate-chandler
meg-gupta
authored and
nate-chandler
committed
Fix mem2reg of lexical enum stack locations
When we have a non trivial enum lexical stack location, and we store a none value, after mem2reg we can end up with an ownership error because of the leaking move_value [lexical]. This change avoids creating move_value [lexical] for such stores. Do the same when we have store_borrow of none values, even though there is no ownership error in this case, handle it like store for consistency
1 parent 73b0ad3 commit 42c1f9c

File tree

2 files changed

+136
-9
lines changed

2 files changed

+136
-9
lines changed

‎lib/SILOptimizer/Transforms/SILMem2Reg.cpp

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ class LiveValues {
9494
/// those usages will be replaced with usages of the stored field. The
9595
/// implementation constructs an instance to match those requirements.
9696
static Owned toReplace(AllocStackInst *asi, SILValue replacement) {
97-
if (lexicalLifetimeEnsured(asi))
97+
if (lexicalLifetimeEnsured(asi) &&
98+
replacement->getOwnershipKind() != OwnershipKind::None)
9899
return {SILValue(), replacement};
99100
return {replacement, SILValue()};
100101
}
@@ -105,9 +106,10 @@ class LiveValues {
105106
if (!lexicalLifetimeEnsured(asi)) {
106107
return stored;
107108
}
108-
// We should have created a move of the @owned stored value.
109-
assert(move);
110-
return move;
109+
assert(move || stored->getOwnershipKind() == OwnershipKind::None);
110+
// If we had a trivial value stored in a lexical non trivial enum, we may
111+
// not have created a move, return stored value in that case.
112+
return move ? move : stored;
111113
}
112114

113115
bool canEndLexicalLifetime() {
@@ -146,7 +148,10 @@ class LiveValues {
146148
// For guaranteed lexical AllocStackInsts--i.e. those that are
147149
// store_borrow locations--we may have created a borrow if the stored
148150
// value is a non-lexical guaranteed value.
149-
assert(isGuaranteedLexicalValue(stored) || borrow);
151+
// We may not have created any borrow, if there was a trivial store to a
152+
// non-trivial lexical enum.
153+
assert(isGuaranteedLexicalValue(stored) || borrow ||
154+
stored->getOwnershipKind() == OwnershipKind::None);
150155
return borrow ? borrow : stored;
151156
}
152157

@@ -655,6 +660,9 @@ static SILValue getLexicalValueForStore(SILInstruction *inst,
655660
if (!lexicalLifetimeEnsured(asi)) {
656661
return SILValue();
657662
}
663+
if (stored->getOwnershipKind() == OwnershipKind::None) {
664+
return SILValue();
665+
}
658666
if (isa<StoreBorrowInst>(inst)) {
659667
if (isGuaranteedLexicalValue(stored)) {
660668
return SILValue();
@@ -679,6 +687,8 @@ static StorageStateTracking<LiveValues>
679687
beginOwnedLexicalLifetimeAfterStore(AllocStackInst *asi, StoreInst *inst) {
680688
assert(lexicalLifetimeEnsured(asi));
681689
SILValue stored = inst->getOperand(CopyLikeInstruction::Src);
690+
assert(stored->getOwnershipKind() == OwnershipKind::Owned);
691+
682692
SILLocation loc = RegularLocation::getAutoGeneratedLocation(inst->getLoc());
683693

684694
MoveValueInst *mvi = nullptr;
@@ -698,6 +708,8 @@ beginGuaranteedLexicalLifetimeAfterStore(AllocStackInst *asi,
698708
StoreBorrowInst *inst) {
699709
assert(lexicalLifetimeEnsured(asi));
700710
SILValue stored = inst->getOperand(CopyLikeInstruction::Src);
711+
assert(stored->getOwnershipKind() != OwnershipKind::None);
712+
701713
SILLocation loc = RegularLocation::getAutoGeneratedLocation(inst->getLoc());
702714

703715
if (isGuaranteedLexicalValue(stored)) {
@@ -1042,7 +1054,9 @@ SILInstruction *StackAllocationPromoter::promoteAllocationInBlock(
10421054
// The current store is now the lastStoreInst (until we see
10431055
// another).
10441056
lastStoreInst = si;
1045-
if (lexicalLifetimeEnsured(asi)) {
1057+
1058+
if (lexicalLifetimeEnsured(asi) &&
1059+
si->getSrc()->getOwnershipKind() != OwnershipKind::None) {
10461060
if (oldRunningVals && oldRunningVals->isStorageValid &&
10471061
canEndLexicalLifetime(oldRunningVals->value)) {
10481062
endOwnedLexicalLifetimeBeforeInst(asi, /*beforeInstruction=*/si, ctx,
@@ -1070,7 +1084,8 @@ SILInstruction *StackAllocationPromoter::promoteAllocationInBlock(
10701084
/*isStorageValid=*/true};
10711085
// The current store is now the lastStoreInst.
10721086
lastStoreInst = sbi;
1073-
if (lexicalLifetimeEnsured(asi)) {
1087+
if (lexicalLifetimeEnsured(asi) &&
1088+
sbi->getSrc()->getOwnershipKind() != OwnershipKind::None) {
10741089
runningVals = beginGuaranteedLexicalLifetimeAfterStore(asi, sbi);
10751090
}
10761091
continue;
@@ -1914,7 +1929,8 @@ void MemoryToRegisters::removeSingleBlockAllocation(AllocStackInst *asi) {
19141929
auto oldRunningVals = runningVals;
19151930
runningVals = {LiveValues::toReplace(asi, /*replacement=*/si->getSrc()),
19161931
/*isStorageValid=*/true};
1917-
if (lexicalLifetimeEnsured(asi)) {
1932+
if (lexicalLifetimeEnsured(asi) &&
1933+
si->getSrc()->getOwnershipKind() != OwnershipKind::None) {
19181934
if (oldRunningVals && oldRunningVals->isStorageValid) {
19191935
endOwnedLexicalLifetimeBeforeInst(asi, /*beforeInstruction=*/si, ctx,
19201936
oldRunningVals->value.getOwned());
@@ -1932,7 +1948,8 @@ void MemoryToRegisters::removeSingleBlockAllocation(AllocStackInst *asi) {
19321948
}
19331949
runningVals = {LiveValues::toReplace(asi, /*replacement=*/sbi->getSrc()),
19341950
/*isStorageValid=*/true};
1935-
if (lexicalLifetimeEnsured(asi)) {
1951+
if (lexicalLifetimeEnsured(asi) &&
1952+
sbi->getSrc()->getOwnershipKind() != OwnershipKind::None) {
19361953
runningVals = beginGuaranteedLexicalLifetimeAfterStore(asi, sbi);
19371954
}
19381955
continue;

‎test/SILOptimizer/mem2reg_ossa_nontrivial.sil

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,3 +1293,113 @@ bb3:
12931293
%t = tuple ()
12941294
return %t : $()
12951295
}
1296+
1297+
// CHECK-LABEL: sil [ossa] @test_storeborrow_trivial_none_to_lexical_storeborrow_only : $@convention(thin) () -> () {
1298+
// CHECK-NOT: alloc_stack
1299+
// CHECK-LABEL: } // end sil function 'test_storeborrow_trivial_none_to_lexical_storeborrow_only'
1300+
sil [ossa] @test_storeborrow_trivial_none_to_lexical_storeborrow_only : $@convention(thin) () -> () {
1301+
bb0:
1302+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1303+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1304+
%sbi = store_borrow %none to %1 : $*FakeOptional<Klass>
1305+
end_borrow %sbi : $*FakeOptional<Klass>
1306+
dealloc_stack %1 : $*FakeOptional<Klass>
1307+
%t = tuple ()
1308+
return %t : $()
1309+
}
1310+
1311+
// CHECK-LABEL: sil [ossa] @test_storeborrow_trivial_none_to_lexical_optional1 : $@convention(thin) () -> () {
1312+
// CHECK-NOT: alloc_stack
1313+
// CHECK-LABEL: } // end sil function 'test_storeborrow_trivial_none_to_lexical_optional1'
1314+
sil [ossa] @test_storeborrow_trivial_none_to_lexical_optional1 : $@convention(thin) () -> () {
1315+
bb0:
1316+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1317+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1318+
%sbi = store_borrow %none to %1 : $*FakeOptional<Klass>
1319+
%ld = load [copy] %sbi : $*FakeOptional<Klass>
1320+
apply undef(%ld) : $@convention(thin) (@owned FakeOptional<Klass>) -> ()
1321+
end_borrow %sbi : $*FakeOptional<Klass>
1322+
dealloc_stack %1 : $*FakeOptional<Klass>
1323+
%t = tuple ()
1324+
return %t : $()
1325+
}
1326+
1327+
// CHECK-LABEL: sil [ossa] @test_storeborrow_trivial_none_to_lexical_optional2 : $@convention(thin) () -> () {
1328+
// CHECK-NOT: alloc_stack
1329+
// CHECK-LABEL: } // end sil function 'test_storeborrow_trivial_none_to_lexical_optional2'
1330+
sil [ossa] @test_storeborrow_trivial_none_to_lexical_optional2 : $@convention(thin) () -> () {
1331+
bb0:
1332+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1333+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1334+
%sbi = store_borrow %none to %1 : $*FakeOptional<Klass>
1335+
%ld = load [copy] %sbi : $*FakeOptional<Klass>
1336+
apply undef(%ld) : $@convention(thin) (@owned FakeOptional<Klass>) -> ()
1337+
br bb1
1338+
1339+
bb1:
1340+
end_borrow %sbi : $*FakeOptional<Klass>
1341+
dealloc_stack %1 : $*FakeOptional<Klass>
1342+
%t = tuple ()
1343+
return %t : $()
1344+
}
1345+
1346+
// CHECK-LABEL: sil [ossa] @test_store_trivial_none_to_lexical_storeonly : $@convention(thin) () -> () {
1347+
// CHECK-NOT: alloc_stack
1348+
// CHECK-LABEL: } // end sil function 'test_store_trivial_none_to_lexical_storeonly'
1349+
sil [ossa] @test_store_trivial_none_to_lexical_storeonly : $@convention(thin) () -> () {
1350+
bb0:
1351+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1352+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1353+
store %none to [trivial] %1 : $*FakeOptional<Klass>
1354+
dealloc_stack %1 : $*FakeOptional<Klass>
1355+
%t = tuple ()
1356+
return %t : $()
1357+
}
1358+
1359+
// CHECK-LABEL: sil [ossa] @test_store_trivial_none_to_lexical_optional1 : $@convention(thin) () -> () {
1360+
// CHECK-NOT: alloc_stack
1361+
// CHECK-LABEL: } // end sil function 'test_store_trivial_none_to_lexical_optional1'
1362+
sil [ossa] @test_store_trivial_none_to_lexical_optional1 : $@convention(thin) () -> () {
1363+
bb0:
1364+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1365+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1366+
store %none to [trivial] %1 : $*FakeOptional<Klass>
1367+
%ld = load [take] %1 : $*FakeOptional<Klass>
1368+
apply undef(%ld) : $@convention(thin) (@owned FakeOptional<Klass>) -> ()
1369+
dealloc_stack %1 : $*FakeOptional<Klass>
1370+
%t = tuple ()
1371+
return %t : $()
1372+
}
1373+
1374+
// CHECK-LABEL: sil [ossa] @test_store_trivial_none_to_lexical_optional2 : $@convention(thin) () -> () {
1375+
// CHECK-NOT: alloc_stack
1376+
// CHECK-LABEL: } // end sil function 'test_store_trivial_none_to_lexical_optional2'
1377+
sil [ossa] @test_store_trivial_none_to_lexical_optional2 : $@convention(thin) () -> () {
1378+
bb0:
1379+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1380+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1381+
store %none to [trivial] %1 : $*FakeOptional<Klass>
1382+
br bb1
1383+
1384+
bb1:
1385+
%ld = load [take] %1 : $*FakeOptional<Klass>
1386+
apply undef(%ld) : $@convention(thin) (@owned FakeOptional<Klass>) -> ()
1387+
dealloc_stack %1 : $*FakeOptional<Klass>
1388+
%t = tuple ()
1389+
return %t : $()
1390+
}
1391+
1392+
// CHECK-LABEL: sil [ossa] @test_store_trivial_none_to_lexical_optional3 : $@convention(thin) () -> () {
1393+
// CHECK-NOT: alloc_stack
1394+
// CHECK-LABEL: } // end sil function 'test_store_trivial_none_to_lexical_optional3'
1395+
sil [ossa] @test_store_trivial_none_to_lexical_optional3 : $@convention(thin) () -> () {
1396+
bb0:
1397+
%1 = alloc_stack [lexical] $FakeOptional<Klass>
1398+
%none = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
1399+
store %none to [init] %1 : $*FakeOptional<Klass>
1400+
%ld = load [take] %1 : $*FakeOptional<Klass>
1401+
apply undef(%ld) : $@convention(thin) (@owned FakeOptional<Klass>) -> ()
1402+
dealloc_stack %1 : $*FakeOptional<Klass>
1403+
%t = tuple ()
1404+
return %t : $()
1405+
}

0 commit comments

Comments
 (0)