Skip to content

Commit 4343d94

Browse files
nate-chandlernate-chandler
authored
Merge pull request #41751 from nate-chandler/mem2reg/bail-on-load-take-complex-projections
[Mem2Reg] Skip load [take] of cast projections.
2 parents 009ed74 + 3508d41 commit 4343d94

File tree

2 files changed

+148
-20
lines changed

2 files changed

+148
-20
lines changed

‎lib/SILOptimizer/Transforms/SILMem2Reg.cpp‎

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1410,19 +1410,12 @@ class MemoryToRegisters {
14101410
/// 1. (load %ASI)
14111411
/// 2. (load (struct_element_addr/tuple_element_addr/unchecked_addr_cast %ASI))
14121412
static bool isAddressForLoad(SILInstruction *load, SILBasicBlock *&singleBlock,
1413-
bool &hasGuaranteedOwnership) {
1414-
1415-
if (isa<LoadInst>(load)) {
1416-
// SILMem2Reg is disabled when we find:
1417-
// (load [take] (struct_element_addr/tuple_element_addr %ASI))
1418-
// struct_element_addr and tuple_element_addr are lowered into
1419-
// struct_extract and tuple_extract and these SIL instructions have a
1420-
// guaranteed ownership. For replacing load's users, we need an owned value.
1421-
// We will need a new copy and destroy of the running val placed after the
1422-
// last use. This is not implemented currently.
1423-
if (hasGuaranteedOwnership &&
1424-
cast<LoadInst>(load)->getOwnershipQualifier() ==
1425-
LoadOwnershipQualifier::Take) {
1413+
bool &involvesUntakableProjection) {
1414+
if (auto *li = dyn_cast<LoadInst>(load)) {
1415+
// SILMem2Reg is disabled when we find a load [take] of an untakable
1416+
// projection. See below for further discussion.
1417+
if (involvesUntakableProjection &&
1418+
li->getOwnershipQualifier() == LoadOwnershipQualifier::Take) {
14261419
return false;
14271420
}
14281421
return true;
@@ -1432,17 +1425,40 @@ static bool isAddressForLoad(SILInstruction *load, SILBasicBlock *&singleBlock,
14321425
!isa<TupleElementAddrInst>(load))
14331426
return false;
14341427

1435-
if (isa<StructElementAddrInst>(load) || isa<TupleElementAddrInst>(load)) {
1436-
hasGuaranteedOwnership = true;
1437-
}
1428+
// None of the projections are lowered to owned values:
1429+
//
1430+
// struct_element_addr and tuple_element_addr instructions are lowered to
1431+
// struct_extract and tuple_extract instructions respectively. These both
1432+
// have guaranteed ownership (since they forward ownership and can only be
1433+
// used on a guaranteed value).
1434+
//
1435+
// unchecked_addr_cast instructions are lowered to unchecked_bitwise_cast
1436+
// instructions. These have unowned ownership.
1437+
//
1438+
// So in no case can a load [take] be lowered into the new projected value
1439+
// (some sequence of struct_extract, tuple_extract, and
1440+
// unchecked_bitwise_cast instructions) taking over ownership of the original
1441+
// value. Without additional changes.
1442+
//
1443+
// For example, for a sequence of element_addr projections could be
1444+
// transformed into a sequence of destructure instructions, followed by a
1445+
// sequence of structure instructions where all the original values are
1446+
// kept in place but the taken value is "knocked out" and replaced with
1447+
// undef. The running value would then be set to the newly structed
1448+
// "knockout" value.
1449+
//
1450+
// Alternatively, a new copy of the running value could be created and a new
1451+
// set of destroys placed after its last uses.
1452+
involvesUntakableProjection = true;
14381453

14391454
// Recursively search for other (non-)loads in the instruction's uses.
1440-
for (auto *use : cast<SingleValueInstruction>(load)->getUses()) {
1455+
auto *svi = cast<SingleValueInstruction>(load);
1456+
for (auto *use : svi->getUses()) {
14411457
SILInstruction *user = use->getUser();
14421458
if (user->getParent() != singleBlock)
14431459
singleBlock = nullptr;
14441460

1445-
if (!isAddressForLoad(user, singleBlock, hasGuaranteedOwnership))
1461+
if (!isAddressForLoad(user, singleBlock, involvesUntakableProjection))
14461462
return false;
14471463
}
14481464
return true;
@@ -1476,8 +1492,8 @@ static bool isCaptured(AllocStackInst *asi, bool &inSingleBlock) {
14761492
singleBlock = nullptr;
14771493

14781494
// Loads are okay.
1479-
bool hasGuaranteedOwnership = false;
1480-
if (isAddressForLoad(user, singleBlock, hasGuaranteedOwnership))
1495+
bool involvesUntakableProjection = false;
1496+
if (isAddressForLoad(user, singleBlock, involvesUntakableProjection))
14811497
continue;
14821498

14831499
// We can store into an AllocStack (but not the pointer).

‎test/SILOptimizer/mem2reg_ossa_nontrivial.sil‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,17 @@ import Swift
77
// Declarations //
88
//////////////////
99

10+
typealias AnyObject = Builtin.AnyObject
11+
1012
class Klass {}
1113

14+
class X {}
15+
16+
struct S {
17+
var v1: AnyObject
18+
var v2: AnyObject
19+
}
20+
1221
struct SmallCodesizeStruct {
1322
var cls1 : Klass
1423
var cls2 : Klass
@@ -40,6 +49,10 @@ public enum FakeOptional<T> {
4049
case none
4150
}
4251

52+
struct UInt8 {
53+
var _value : Builtin.Int8
54+
}
55+
4356
sil [ossa] @get_nontrivialstruct : $@convention(thin) () -> @owned NonTrivialStruct
4457
sil [ossa] @get_nontrivialenum : $@convention(thin) () -> @owned NonTrivialEnum
4558
sil [ossa] @get_optionalnontrivialstruct : $@convention(thin) () -> @owned FakeOptional<NonTrivialStruct>
@@ -1114,3 +1127,102 @@ bb9:
11141127
return %res : $()
11151128
}
11161129

1130+
// Test that a load [take] of an unchecked_addr_cast doesn't get promoted.
1131+
//
1132+
// CHECK-LABEL: sil [ossa] @load_take_unchecked_addr_cast : {{.*}} {
1133+
// CHECK: load [take]
1134+
// CHECK-LABEL: } // end sil function 'load_take_unchecked_addr_cast'
1135+
sil [ossa] @load_take_unchecked_addr_cast : $@convention(thin) (@guaranteed AnyObject) -> () {
1136+
entry(%instance : @guaranteed $AnyObject):
1137+
%copy = copy_value %instance : $AnyObject
1138+
%storage = alloc_stack $AnyObject
1139+
store %copy to [init] %storage : $*AnyObject
1140+
%cast_addr = unchecked_addr_cast %storage : $*AnyObject to $*Klass
1141+
%value = load [take] %cast_addr : $*Klass
1142+
dealloc_stack %storage : $*AnyObject
1143+
destroy_value %value : $Klass
1144+
%retval = tuple ()
1145+
return %retval : $()
1146+
}
1147+
1148+
// Don't bail if the original address is destroyed even if we see a cast.
1149+
//
1150+
// CHECK-LABEL: sil [ossa] @destroy_original_storage : {{.*}} {
1151+
// CHECK: destroy_value
1152+
// CHECK-LABEL: } // end sil function 'destroy_original_storage'
1153+
sil [ossa] @destroy_original_storage : $@convention(thin) (@guaranteed AnyObject) -> () {
1154+
entry(%instance : @guaranteed $AnyObject):
1155+
%copy = copy_value %instance : $AnyObject
1156+
%storage = alloc_stack $AnyObject
1157+
store %copy to [init] %storage : $*AnyObject
1158+
%cast_addr = unchecked_addr_cast %storage : $*AnyObject to $*Klass
1159+
%value = load [copy] %cast_addr : $*Klass
1160+
destroy_addr %storage : $*AnyObject
1161+
dealloc_stack %storage : $*AnyObject
1162+
destroy_value %value : $Klass
1163+
%retval = tuple ()
1164+
return %retval : $()
1165+
}
1166+
1167+
// Bail if the address produced by an unchecked_addr_cast is destroyed.
1168+
//
1169+
// CHECK-LABEL: sil [ossa] @destroy_addr_unchecked_addr_cast : {{.*}} {
1170+
// CHECK: unchecked_addr_cast
1171+
// CHECK: load [copy]
1172+
// CHECK-LABEL: } // end sil function 'destroy_addr_unchecked_addr_cast'
1173+
sil [ossa] @destroy_addr_unchecked_addr_cast : $@convention(thin) (@guaranteed AnyObject) -> () {
1174+
entry(%instance : @guaranteed $AnyObject):
1175+
%copy = copy_value %instance : $AnyObject
1176+
%storage = alloc_stack $AnyObject
1177+
store %copy to [init] %storage : $*AnyObject
1178+
%cast_addr = unchecked_addr_cast %storage : $*AnyObject to $*Klass
1179+
%value = load [copy] %cast_addr : $*Klass
1180+
destroy_addr %cast_addr : $*Klass
1181+
dealloc_stack %storage : $*AnyObject
1182+
destroy_value %value : $Klass
1183+
%retval = tuple ()
1184+
return %retval : $()
1185+
}
1186+
1187+
// Bail if there's a load [take] of one of multiple non-trivial fields.
1188+
//
1189+
// CHECK-LABEL: sil [ossa] @load_take_one_of_two_nontrivial_struct_fields : {{.*}} {
1190+
// CHECK: load [take]
1191+
// CHECK: destroy_addr
1192+
// CHECK-LABEL: } // end sil function 'load_take_one_of_two_nontrivial_struct_fields'
1193+
sil [ossa] @load_take_one_of_two_nontrivial_struct_fields : $@convention(thin) (@guaranteed S) -> () {
1194+
entry(%instance : @guaranteed $S):
1195+
%copy = copy_value %instance : $S
1196+
%storage = alloc_stack $S
1197+
store %copy to [init] %storage : $*S
1198+
%v1_addr = struct_element_addr %storage : $*S, #S.v1
1199+
%cast_addr = unchecked_addr_cast %v1_addr : $*AnyObject to $*Klass
1200+
%value = load [take] %cast_addr : $*Klass
1201+
%v2_addr = struct_element_addr %storage : $*S, #S.v2
1202+
destroy_addr %v2_addr : $*AnyObject
1203+
//destroy_addr %storage : $*S
1204+
dealloc_stack %storage : $*S
1205+
destroy_value %value : $Klass
1206+
%retval = tuple ()
1207+
return %retval : $()
1208+
}
1209+
1210+
// Don't bail if we happen to see an unchecked_addr_cast but then load [take]
1211+
// the original address.
1212+
//
1213+
// CHECK-LABEL: sil [ossa] @load_take_original_despite_cast : {{.*}} {
1214+
// CHECK: unchecked_bitwise_cast
1215+
// CHECK: destroy_value
1216+
// CHECK-LABEL: } // end sil function 'load_take_original_despite_cast'
1217+
sil [ossa] @load_take_original_despite_cast : $@convention(thin) (@owned AnyObject) -> () {
1218+
entry(%instance : @owned $AnyObject):
1219+
%74 = alloc_stack $AnyObject
1220+
store %instance to [init] %74 : $*AnyObject
1221+
%76 = unchecked_addr_cast %74 : $*AnyObject to $*UInt8
1222+
%77 = load [trivial] %76 : $*UInt8
1223+
%79 = load [take] %74 : $*AnyObject
1224+
destroy_value %79 : $AnyObject
1225+
dealloc_stack %74 : $*AnyObject
1226+
%82 = tuple ()
1227+
return %82 : $()
1228+
}

0 commit comments

Comments
 (0)