Skip to content

Commit be47582

Browse files
gottesmmgottesmm
committed
[pmo] Move handling of releases: ElementUseCollector::{collectFrom,collectContainerUses}()
Since: 1. We only handle alloc_stack, alloc_box in predictable memopts. 2. alloc_stack can not be released. We know that the release collecting in collectFrom can just be done in collectContainerUses() [which only processes alloc_box]. This also let me simplify some code as well and add a defensive check in case for some reason we are passed a release_value on the box. NOTE: I verified that previously this did not result in a bug since we would consider the release_value to be an escape of the underlying value even though we didn't handle it in collectFrom. But the proper way to handle release_value is like strong_release, so I added code to do that as well.
1 parent b3b0ea9 commit be47582

File tree

2 files changed

+95
-30
lines changed

2 files changed

+95
-30
lines changed

‎lib/SILOptimizer/Mandatory/PMOMemoryUseCollector.cpp

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -123,44 +123,52 @@ class ElementUseCollector {
123123
} // end anonymous namespace
124124

125125
bool ElementUseCollector::collectFrom() {
126-
bool shouldOptimize = false;
127-
128-
if (auto *ABI = TheMemory.getContainer()) {
129-
shouldOptimize = collectContainerUses(ABI);
130-
} else {
131-
shouldOptimize = collectUses(TheMemory.getAddress());
132-
}
133-
134-
if (!shouldOptimize)
135-
return false;
136-
137-
// Collect information about the retain count result as well.
138-
for (auto *op : TheMemory.MemoryInst->getUses()) {
139-
auto *user = op->getUser();
140-
141-
// If this is a strong_release, stash it.
142-
if (isa<StrongReleaseInst>(user)) {
143-
Releases.push_back(user);
144-
}
126+
if (auto *abi = TheMemory.getContainer()) {
127+
return collectContainerUses(abi);
145128
}
146129

147-
return true;
130+
return collectUses(TheMemory.getAddress());
148131
}
149132

150-
bool ElementUseCollector::collectContainerUses(AllocBoxInst *ABI) {
151-
for (Operand *UI : ABI->getUses()) {
152-
auto *User = UI->getUser();
133+
bool ElementUseCollector::collectContainerUses(AllocBoxInst *abi) {
134+
for (auto *ui : abi->getUses()) {
135+
auto *user = ui->getUser();
153136

154-
// Deallocations and retain/release don't affect the value directly.
155-
if (isa<DeallocBoxInst>(User))
137+
// dealloc_box deallocated a box containing uninitialized memory. This can
138+
// not effect any value stored into the box.
139+
if (isa<DeallocBoxInst>(user))
156140
continue;
157-
if (isa<StrongRetainInst>(User))
141+
142+
// Retaining the box doesn't effect the value inside the box.
143+
if (isa<StrongRetainInst>(user) || isa<RetainValueInst>(user))
158144
continue;
159-
if (isa<StrongReleaseInst>(User))
145+
146+
// Since we are trying to promote loads/stores, any releases of the box are
147+
// not considered uses of the underlying value due to:
148+
//
149+
// 1. If this is not the last release of the box, then the underlying value
150+
// is not effected implying we do not add this value.
151+
//
152+
// 2. If this is the last release of the box, then the box's destruction
153+
// will result in a release of the underlying value. If there are any
154+
// loads/stores after this point, the behavior would be undefined so we can
155+
// ignore this possibility.
156+
//
157+
// That being said, if we want to eliminate the box completely we need to
158+
// know where the releases are so that we can release the value that would
159+
// have been at +1 in the box at that time. So we add these to the Releases
160+
// array.
161+
//
162+
// FIXME: Since we do not support promoting strong_release or release_value
163+
// today this will cause the underlying allocation to never be
164+
// eliminated. That should be implemented and fixed.
165+
if (isa<StrongReleaseInst>(user) || isa<ReleaseValueInst>(user)) {
166+
Releases.push_back(user);
160167
continue;
168+
}
161169

162-
if (auto project = dyn_cast<ProjectBoxInst>(User)) {
163-
if (!collectUses(project))
170+
if (auto *p = dyn_cast<ProjectBoxInst>(user)) {
171+
if (!collectUses(p))
164172
return false;
165173
continue;
166174
}
@@ -170,7 +178,7 @@ bool ElementUseCollector::collectContainerUses(AllocBoxInst *ABI) {
170178
//
171179
// This will cause the dataflow to stop propagating any information at the
172180
// use block.
173-
Uses.emplace_back(User, PMOUseKind::Escape);
181+
Uses.emplace_back(user, PMOUseKind::Escape);
174182
}
175183

176184
return true;

‎test/SILOptimizer/predictable_memopt.sil

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,63 @@ bb0(%0 : $Int):
4343
return %d : $Int
4444
}
4545

46+
// In this example we create two boxes. The first box is initialized and then
47+
// taken from to initialize the second box. This means that the first box must
48+
// be dealloc_boxed (since its underlying memory is considered invalid). In
49+
// contrast, the 2nd box must be released so that we destroy the underlying
50+
// input object.
51+
//
52+
// CHECK-LABEL: sil @simple_reg_promotion_nontrivial_memory : $@convention(thin) (@owned Builtin.NativeObject) -> () {
53+
// CHECK: strong_release
54+
// CHECK-NOT: dealloc_box
55+
// CHECK: } // end sil function 'simple_reg_promotion_nontrivial_memory'
56+
sil @simple_reg_promotion_nontrivial_memory : $@convention(thin) (@owned Builtin.NativeObject) -> () {
57+
bb0(%0 : $Builtin.NativeObject):
58+
%1 = alloc_box $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
59+
%1a = project_box %1 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>, 0
60+
store %0 to %1a : $*Builtin.NativeObject
61+
62+
%3 = alloc_box $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
63+
%3a = project_box %3 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>, 0
64+
%4 = load %1a : $*Builtin.NativeObject
65+
store %4 to %3a : $*Builtin.NativeObject
66+
strong_release %3 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
67+
dealloc_box %1 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
68+
69+
%9999 = tuple()
70+
return %9999 : $()
71+
}
72+
73+
// Same as the last test but with release_value to be defensive in the cast that
74+
// someone passes us such SIL.
75+
//
76+
// CHECK-LABEL: sil @simple_reg_promotion_nontrivial_memory_release_value : $@convention(thin) (@owned Builtin.NativeObject) -> () {
77+
// The retain value is on the dealloc_box.
78+
// CHECK-NOT: retain_value
79+
// CHECK: release_value
80+
// CHECK-NOT: dealloc_box
81+
// CHECK: } // end sil function 'simple_reg_promotion_nontrivial_memory_release_value'
82+
sil @simple_reg_promotion_nontrivial_memory_release_value : $@convention(thin) (@owned Builtin.NativeObject) -> () {
83+
bb0(%0 : $Builtin.NativeObject):
84+
%1 = alloc_box $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
85+
%1a = project_box %1 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>, 0
86+
store %0 to %1a : $*Builtin.NativeObject
87+
88+
// Also verify that we skip a retain_value on the dealloc_box.
89+
retain_value %1 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
90+
91+
%3 = alloc_box $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
92+
%3a = project_box %3 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>, 0
93+
%4 = load %1a : $*Builtin.NativeObject
94+
store %4 to %3a : $*Builtin.NativeObject
95+
release_value %3 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
96+
dealloc_box %1 : $<τ_0_0> { var τ_0_0 } <Builtin.NativeObject>
97+
98+
%9999 = tuple()
99+
return %9999 : $()
100+
}
101+
102+
46103
sil @takes_Int_inout : $@convention(thin) (@inout Int) -> ()
47104
sil @takes_NativeObject_inout : $@convention(thin) (@inout Builtin.NativeObject) -> ()
48105

0 commit comments

Comments
 (0)