Skip to content

Commit d881f92

Browse files
committed
[OSSACanonicalizeOwned] Only discover defs once.
The utility performs two def-use traversals. The first determines liveness from uses. The second rewrites copies. Previously, the defs whose uses were analyzed were discovered twice, once during each traversal. The non-triviality of the discovery logic (i.e. the logic determining when to walk into the values produced by the instructions which were the users of visited uses) opened the possibility for a divergence between the two discoveries. This possibility had indeed been realized--the two traversals didn't visit exactly the same uses, and issues ensue. Here, the defs whose uses are analyzed are discovered only once (and not discarded as their uses are analyzed) during the first traversal. The second traversal reuses the defs discovered in the first traversal, eliminating the possibility of a def discovery difference. The second traversal is now done in a different order. This results in perturbing the SIL in certain cases.
1 parent 5ac7226 commit d881f92

File tree

2 files changed

+3
-22
lines changed

2 files changed

+3
-22
lines changed

lib/SILOptimizer/Utils/CanonicalizeOSSALifetime.cpp

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1161,16 +1161,7 @@ void CanonicalizeOSSALifetime::rewriteCopies(
11611161
assert(getCurrentDef()->getOwnershipKind() == OwnershipKind::Owned);
11621162

11631163
// Shadow discoveredDefs in order to constrain its uses.
1164-
auto &discoveredDefs = this->discoveredDefs;
1165-
1166-
SmallVector<unsigned, 8> indexWorklist;
1167-
ValueSet visitedDefs(getCurrentDef()->getFunction());
1168-
auto addDefToWorklist = [&](Def def) {
1169-
if (!visitedDefs.insert(def.getValue()))
1170-
return;
1171-
discoveredDefs.push_back(def);
1172-
indexWorklist.push_back(discoveredDefs.size() - 1);
1173-
};
1164+
const auto &discoveredDefs = this->discoveredDefs;
11741165

11751166
InstructionSetVector instsToDelete(getCurrentDef()->getFunction());
11761167

@@ -1180,11 +1171,6 @@ void CanonicalizeOSSALifetime::rewriteCopies(
11801171
// it requires a copy.
11811172
auto visitUse = [&](Operand *use) {
11821173
auto *user = use->getUser();
1183-
// Recurse through copies.
1184-
if (auto *copy = dyn_cast<CopyValueInst>(user)) {
1185-
addDefToWorklist(Def::copy(copy));
1186-
return true;
1187-
}
11881174
if (destroys.contains(user)) {
11891175
auto *destroy = cast<DestroyValueInst>(user);
11901176
// If this destroy was marked as a final destroy, ignore it; otherwise,
@@ -1217,13 +1203,8 @@ void CanonicalizeOSSALifetime::rewriteCopies(
12171203
return true;
12181204
};
12191205

1220-
discoveredDefs.clear();
1221-
addDefToWorklist(Def::root(getCurrentDef()));
12221206
// Perform a def-use traversal, visiting each use operand.
1223-
1224-
while (!indexWorklist.empty()) {
1225-
auto index = indexWorklist.pop_back_val();
1226-
auto def = discoveredDefs[index];
1207+
for (auto def : discoveredDefs) {
12271208
switch (def) {
12281209
case Def::Kind::BorrowedFrom:
12291210
case Def::Kind::Reborrow:

test/SILOptimizer/copy_propagation_opaque.sil

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ bb(%0 : @owned $T):
242242
// CHECK-ONONE-NEXT: [[CP:%.*]] = copy_value [[INSTANCE]] : $T
243243
// CHECK-NEXT: // function_ref takeMultipleOwned
244244
// CHECK-NEXT: function_ref @takeMultipleOwned : $@convention(thin) <τ_0_0> (@in τ_0_0, @in τ_0_0) -> ()
245-
// CHECK-NEXT: apply %{{.*}}<T>([[INSTANCE]], [[CP]]) : $@convention(thin) <τ_0_0> (@in τ_0_0, @in τ_0_0) -> ()
245+
// CHECK-NEXT: apply %{{.*}}<T>([[CP]], [[INSTANCE]]) : $@convention(thin) <τ_0_0> (@in τ_0_0, @in τ_0_0) -> ()
246246
// CHECK-ONONE-NEXT: destroy_value
247247
// CHECK-NEXT: tuple ()
248248
// CHECK-NEXT: return

0 commit comments

Comments
 (0)