Skip to content

Commit fb01580

Browse files
nate-chandlernate-chandler
authored
Merge pull request #62873 from nate-chandler/bug/20230105/1
[SSADestroyHoisting] Don't fold over trivial use.
2 parents 974a6bd + 4dec48f commit fb01580

File tree

3 files changed

+104
-12
lines changed

3 files changed

+104
-12
lines changed

‎include/swift/SIL/MemAccessUtils.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,8 @@ class AccessPath {
11321132
// Precondition: this != subNode
11331133
PathNode findPrefix(PathNode subNode) const;
11341134

1135+
bool isPrefixOf(PathNode other) { return node->isPrefixOf(other.node); }
1136+
11351137
bool operator==(PathNode other) const { return node == other.node; }
11361138
bool operator!=(PathNode other) const { return node != other.node; }
11371139
};

‎lib/SILOptimizer/Transforms/SSADestroyHoisting.cpp

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,7 @@ class HoistDestroys {
441441
SmallVectorImpl<LoadInst *> &loads,
442442
SmallVectorImpl<CopyAddrInst *> &copies,
443443
SmallPtrSetImpl<AccessPath::PathNode> &leaves,
444+
SmallPtrSetImpl<AccessPath::PathNode> &trivialLeaves,
444445
const AccessStorage &storage,
445446
const DeinitBarriers &deinitBarriers);
446447

@@ -568,6 +569,25 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
568569
// destroy_addr in order for folding to occur.
569570
llvm::SmallPtrSet<AccessPath::PathNode, 16> leaves;
570571

572+
// The trivial storage leaves of the root storage. They needn't be destroyed
573+
// in the sequence prior to the destroy_addr, but their uses may obstruct
574+
// folding. For example, given an %object and %triv a trivial subobject
575+
//
576+
// load [copy] %object
577+
// load [trivial] %triv
578+
// destroy_addr %object
579+
//
580+
// it isn't legal to fold the destroy_addr into the load of %object like
581+
//
582+
// load [take] %object
583+
// load [trivial] %triv
584+
//
585+
// because the memory location %triv is no longer valid. In general, it would
586+
// be fine to support folding over accesses of trivial subobjects so long as
587+
// they occur prior to the access to some nontrivial subobject that contains
588+
// it.
589+
SmallPtrSet<AccessPath::PathNode, 16> trivialLeaves;
590+
571591
visitProductLeafAccessPathNodes(storageRoot, typeExpansionContext, module,
572592
[&](AccessPath::PathNode node, SILType ty) {
573593
if (ty.isTrivial(*function))
@@ -577,8 +597,8 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
577597

578598
for (auto *instruction = barrier; instruction != nullptr;
579599
instruction = instruction->getPreviousInstruction()) {
580-
if (checkFoldingBarrier(instruction, loads, copies, leaves, storage,
581-
deinitBarriers))
600+
if (checkFoldingBarrier(instruction, loads, copies, leaves, trivialLeaves,
601+
storage, deinitBarriers))
582602
return false;
583603

584604
// If we have load [copy]s or copy_addrs of projections out of the root
@@ -672,8 +692,9 @@ bool HoistDestroys::foldBarrier(SILInstruction *barrier,
672692
bool HoistDestroys::checkFoldingBarrier(
673693
SILInstruction *instruction, SmallVectorImpl<LoadInst *> &loads,
674694
SmallVectorImpl<CopyAddrInst *> &copies,
675-
SmallPtrSetImpl<AccessPath::PathNode> &leaves, const AccessStorage &storage,
676-
const DeinitBarriers &deinitBarriers) {
695+
SmallPtrSetImpl<AccessPath::PathNode> &leaves,
696+
SmallPtrSetImpl<AccessPath::PathNode> &trivialLeaves,
697+
const AccessStorage &storage, const DeinitBarriers &deinitBarriers) {
677698
// The address of a projection out of the root storage which would be
678699
// folded if folding is possible.
679700
//
@@ -720,14 +741,18 @@ bool HoistDestroys::checkFoldingBarrier(
720741
// Find its nontrivial product leaves and remove them from the set of
721742
// leaves of the root storage which we're wating to see.
722743
bool alreadySawLeaf = false;
723-
visitProductLeafAccessPathNodes(address, typeExpansionContext, module,
724-
[&](AccessPath::PathNode node, SILType ty) {
725-
if (ty.isTrivial(*function))
726-
return;
727-
bool erased = leaves.erase(node);
728-
alreadySawLeaf =
729-
alreadySawLeaf || !erased;
730-
});
744+
bool alreadySawTrivialSubleaf = false;
745+
visitProductLeafAccessPathNodes(
746+
address, typeExpansionContext, module,
747+
[&](AccessPath::PathNode node, SILType ty) {
748+
if (ty.isTrivial(*function)) {
749+
bool inserted = !trivialLeaves.insert(node).second;
750+
alreadySawTrivialSubleaf = alreadySawTrivialSubleaf || inserted;
751+
return;
752+
}
753+
bool erased = leaves.erase(node);
754+
alreadySawLeaf = alreadySawLeaf || !erased;
755+
});
731756
if (alreadySawLeaf) {
732757
// We saw this non-trivial product leaf already. That means there are
733758
// multiple load [copy]s or copy_addrs of at least one product leaf
@@ -736,6 +761,11 @@ bool HoistDestroys::checkFoldingBarrier(
736761
// Give up on folding.
737762
return true;
738763
}
764+
if (alreadySawTrivialSubleaf) {
765+
// We saw this trivial leaf already. That means there was some later
766+
// load [copy] or copy_addr of it. Give up on folding.
767+
return true;
768+
}
739769
} else if (deinitBarriers.isBarrier(instruction)) {
740770
// We didn't find an instruction that was both
741771
// - relevant (i.e. a copy_addr or a load [take])

‎test/SILOptimizer/hoist_destroy_addr.sil

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ struct I {
6060
var value: Builtin.Int64
6161
}
6262

63+
struct Slice {
64+
var _startIndex: I
65+
var _guts: AnyObject
66+
}
67+
6368
typealias TXXI = (X, X, I)
6469

6570
struct SXXI {
@@ -1073,6 +1078,61 @@ entry(%instance : @owned $AnyObject):
10731078
return %cast : $X
10741079
}
10751080

1081+
// Don't fold a destroy_addr into a copy_addr of the whole aggregate if there
1082+
// is an access to some trivial subobject afterwards.
1083+
// CHECK-LABEL: sil [ossa] @nofold_destroy_addr_into_whole_with_later_trivial_subobject_use : {{.*}} {
1084+
// CHECK: destroy_addr
1085+
// CHECK-LABEL: } // end sil function 'nofold_destroy_addr_into_whole_with_later_trivial_subobject_use'
1086+
sil [ossa] @nofold_destroy_addr_into_whole_with_later_trivial_subobject_use : $@convention(thin) (@in Slice) -> (@out Slice) {
1087+
bb0(%out : $*Slice, %4 : $*Slice):
1088+
copy_addr %4 to [init] %out : $*Slice
1089+
%8 = alloc_stack $I
1090+
%9 = struct_element_addr %4 : $*Slice, #Slice._startIndex
1091+
copy_addr %9 to [init] %8 : $*I
1092+
destroy_addr %4 : $*Slice
1093+
dealloc_stack %8 : $*I
1094+
%19 = tuple ()
1095+
return %19 : $()
1096+
}
1097+
1098+
// Like fold_struct_enclosing_tuples_leaf_fields_mixed_insts but with an access
1099+
// of a trivial field afterwards.
1100+
// CHECK-LABEL: sil [ossa] @nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject : {{.*}} {
1101+
// CHECK: load [trivial]
1102+
// CHECK: destroy_addr {{%[^,]+}} : $*STXXITXXII
1103+
// CHECK-LABEL: } // end sil function 'nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject'
1104+
sil [ossa] @nofold_struct_enclosing_tuples_leaf_fields_mixed_insts_with_later_trivial_subobject : $@convention(thin) (@owned STXXITXXII) -> @owned (X, X) {
1105+
entry(%instance : @owned $STXXITXXII):
1106+
%addr = alloc_stack $STXXITXXII
1107+
%txxi_1_alone = alloc_stack $X
1108+
%txxi_2_alone = alloc_stack $X
1109+
store %instance to [init] %addr : $*STXXITXXII
1110+
%txxi_1_addr = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.txxi_1
1111+
%txxi_2_addr = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.txxi_2
1112+
%txxi_1_x_0_addr = tuple_element_addr %txxi_1_addr : $*(X, X, I), 0
1113+
%txxi_1_x_1_addr = tuple_element_addr %txxi_1_addr : $*(X, X, I), 1
1114+
%txxi_2_x_0_addr = tuple_element_addr %txxi_2_addr : $*(X, X, I), 0
1115+
%txxi_2_x_1_addr = tuple_element_addr %txxi_2_addr : $*(X, X, I), 1
1116+
%trivial = struct_element_addr %addr : $*STXXITXXII, #STXXITXXII.i
1117+
1118+
copy_addr %txxi_1_x_0_addr to [init] %txxi_1_alone : $*X
1119+
%x1x1 = load [copy] %txxi_1_x_1_addr : $*X
1120+
copy_addr %txxi_2_x_0_addr to [init] %txxi_2_alone : $*X
1121+
%x2x1 = load [copy] %txxi_2_x_1_addr : $*X
1122+
%i = load [trivial] %trivial : $*I
1123+
destroy_addr %addr : $*STXXITXXII
1124+
%barrier = function_ref @unknown : $@convention(thin) () -> ()
1125+
apply %barrier() : $@convention(thin) () -> ()
1126+
1127+
destroy_addr %txxi_1_alone : $*X
1128+
destroy_addr %txxi_2_alone : $*X
1129+
dealloc_stack %txxi_2_alone : $*X
1130+
dealloc_stack %txxi_1_alone : $*X
1131+
dealloc_stack %addr : $*STXXITXXII
1132+
%retval = tuple (%x1x1 : $X, %x2x1 : $X)
1133+
return %retval : $(X, X)
1134+
}
1135+
10761136
// CHECK-LABEL: sil [ossa] @hoist_over_apply_of_non_barrier_fn : {{.*}} {
10771137
// CHECK: destroy_addr
10781138
// CHECK: apply

0 commit comments

Comments
 (0)