Skip to content

Commit d8870fc

Browse files
committed
[semantic-arc-opts] Add a worklist.
We first build up the worklist by applying our visitor to the CFG via a simple instruction walk. During this walk, we know that we will only ever modify instructions that are dominated by the visited instruction since we do not look through basic block arguments. Then once we have finished traversing the CFG, optimizing and gathering opportunities, we drain the worklist. This is eventually where we should process block arguments that we may be able to optimize.
1 parent a2d383e commit d8870fc

File tree

2 files changed

+151
-20
lines changed

2 files changed

+151
-20
lines changed

lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp

Lines changed: 128 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "swift/SIL/BranchPropagatedUser.h"
1717
#include "swift/SIL/MemAccessUtils.h"
1818
#include "swift/SIL/OwnershipUtils.h"
19+
#include "swift/SILOptimizer/Utils/Local.h"
1920
#include "swift/SIL/SILArgument.h"
2021
#include "swift/SIL/SILBuilder.h"
2122
#include "swift/SIL/SILInstruction.h"
@@ -121,8 +122,23 @@ static bool isConsumed(
121122

122123
namespace {
123124

125+
/// A two stage visitor that optimizes ownership instructions and eliminates any
126+
/// trivially dead code that results after optimization. The two stages are used
127+
/// to avoid iterator invalidation. Specifically:
128+
///
129+
/// 1. We first process the CFG instruction by instruction, only eliminating
130+
/// instructions that are guaranteed to be dominated by the visited
131+
/// instrution. While we do that, we add the operands of any instruction that we
132+
/// successfully optimize to the worklist. NOTE: We do not process arguments
133+
/// here to get SSA guarantees around dominance.
134+
///
135+
/// 2. Once we have processed the CFG and done some initial optimization, we
136+
/// enter phase 2 where we process the worklist. Here we are allowed to process
137+
/// arbitrary values and instructions, removing things that we are erasing from
138+
/// the worklist before we delete them.
124139
struct SemanticARCOptVisitor
125140
: SILInstructionVisitor<SemanticARCOptVisitor, bool> {
141+
SmallSetVector<SILValue, 32> worklist;
126142
SILFunction &F;
127143
Optional<DeadEndBlocks> TheDeadEndBlocks;
128144

@@ -133,17 +149,100 @@ struct SemanticARCOptVisitor
133149
TheDeadEndBlocks.emplace(&F);
134150
return *TheDeadEndBlocks;
135151
}
136-
152+
153+
/// Given a single value instruction, RAUW it with newValue, add newValue to
154+
/// the worklist, and then call eraseInstruction on i.
155+
void eraseAndRAUWSingleValueInstruction(SingleValueInstruction *i, SILValue newValue) {
156+
worklist.insert(newValue);
157+
i->replaceAllUsesWith(newValue);
158+
eraseInstruction(i);
159+
}
160+
161+
/// Add all operands of i to the worklist and then call eraseInstruction on
162+
/// i. Assumes that the instruction doesnt have users.
163+
void eraseInstructionAndAddOptsToWorklist(SILInstruction *i) {
164+
// Then copy all operands into the worklist for future processing.
165+
for (SILValue v : i->getOperandValues()) {
166+
worklist.insert(v);
167+
}
168+
eraseInstruction(i);
169+
}
170+
171+
/// Remove all results of the given instruction from the worklist and then
172+
/// erase the instruction. Assumes that the instruction does not have any
173+
/// users left.
174+
void eraseInstruction(SILInstruction *i) {
175+
// Remove all SILValues of the instruction from the worklist and then erase
176+
// the instruction.
177+
for (SILValue result : i->getResults()) {
178+
worklist.remove(result);
179+
}
180+
i->eraseFromParent();
181+
}
182+
183+
/// The default visitor.
137184
bool visitSILInstruction(SILInstruction *i) { return false; }
138185
bool visitCopyValueInst(CopyValueInst *cvi);
139186
bool visitBeginBorrowInst(BeginBorrowInst *bbi);
140187
bool visitLoadInst(LoadInst *li);
141188

142189
bool isWrittenTo(LoadInst *li);
190+
191+
bool processWorklist();
192+
193+
bool performGuaranteedCopyValueOptimization(CopyValueInst *cvi);
194+
bool eliminateDeadLiveRangeCopyValue(CopyValueInst *cvi);
143195
};
144196

145197
} // end anonymous namespace
146198

199+
bool SemanticARCOptVisitor::processWorklist() {
200+
// NOTE: The madeChange here is not strictly necessary since we only have
201+
// items added to the worklist today if we have already made /some/ sort of
202+
// change. That being said, I think there is a low cost to including this here
203+
// and makes the algorithm more correct, visually and in the face of potential
204+
// refactoring.
205+
bool madeChange = false;
206+
207+
while (!worklist.empty()) {
208+
SILValue next = worklist.pop_back_val();
209+
210+
// First check if this is an instruction that is trivially dead. This can
211+
// occur if we eliminate rr traffic resulting in dead projections and the
212+
// like.
213+
//
214+
// If we delete, we first add all of our deleted instructions operands to
215+
// the worklist and then remove all results (since we are going to delete
216+
// the instruction).
217+
if (auto *defInst = next->getDefiningInstruction()) {
218+
if (isInstructionTriviallyDead(defInst)) {
219+
madeChange = true;
220+
recursivelyDeleteTriviallyDeadInstructions(
221+
defInst, true/*force*/,
222+
[&](SILInstruction *i) {
223+
for (SILValue operand : i->getOperandValues()) {
224+
worklist.insert(operand);
225+
}
226+
for (SILValue result : i->getResults()) {
227+
worklist.remove(result);
228+
}
229+
++NumEliminatedInsts;
230+
});
231+
continue;
232+
}
233+
}
234+
235+
// Otherwise, if we have a single value instruction (to be expanded later
236+
// perhaps), try to visit that value recursively.
237+
if (auto *svi = dyn_cast<SingleValueInstruction>(next)) {
238+
madeChange |= visit(svi);
239+
continue;
240+
}
241+
}
242+
243+
return madeChange;
244+
}
245+
147246
bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) {
148247
auto kind = bbi->getOperand().getOwnershipKind();
149248
SmallVector<EndBorrowInst *, 16> endBorrows;
@@ -167,11 +266,11 @@ bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) {
167266
// begin borrow and end borrows.
168267
while (!endBorrows.empty()) {
169268
auto *ebi = endBorrows.pop_back_val();
170-
ebi->eraseFromParent();
269+
eraseInstruction(ebi);
171270
++NumEliminatedInsts;
172271
}
173-
bbi->replaceAllUsesWith(bbi->getOperand());
174-
bbi->eraseFromParent();
272+
273+
eraseAndRAUWSingleValueInstruction(bbi, bbi->getOperand());
175274
++NumEliminatedInsts;
176275
return true;
177276
}
@@ -216,7 +315,7 @@ static bool canHandleOperand(SILValue operand, SmallVectorImpl<SILValue> &out) {
216315
// are within the borrow scope.
217316
//
218317
// TODO: This needs a better name.
219-
static bool performGuaranteedCopyValueOptimization(CopyValueInst *cvi) {
318+
bool SemanticARCOptVisitor::performGuaranteedCopyValueOptimization(CopyValueInst *cvi) {
220319
SmallVector<SILValue, 16> borrowIntroducers;
221320

222321
// Whitelist the operands that we know how to support and make sure
@@ -250,11 +349,11 @@ static bool performGuaranteedCopyValueOptimization(CopyValueInst *cvi) {
250349
// within the guaranteed value scope. First delete the destroys/copies.
251350
while (!destroys.empty()) {
252351
auto *dvi = destroys.pop_back_val();
253-
dvi->eraseFromParent();
352+
eraseInstruction(dvi);
254353
++NumEliminatedInsts;
255354
}
256-
cvi->replaceAllUsesWith(cvi->getOperand());
257-
cvi->eraseFromParent();
355+
356+
eraseAndRAUWSingleValueInstruction(cvi, cvi->getOperand());
258357

259358
// Then change all of our guaranteed forwarding insts to have guaranteed
260359
// ownership kind instead of what ever they previously had (ignoring trivial
@@ -302,12 +401,12 @@ static bool performGuaranteedCopyValueOptimization(CopyValueInst *cvi) {
302401

303402
/// If cvi only has destroy value users, then cvi is a dead live range. Lets
304403
/// eliminate all such dead live ranges.
305-
static bool eliminateDeadLiveRangeCopyValue(CopyValueInst *cvi) {
404+
bool SemanticARCOptVisitor::eliminateDeadLiveRangeCopyValue(CopyValueInst *cvi) {
306405
// See if we are lucky and have a simple case.
307406
if (auto *op = cvi->getSingleUse()) {
308407
if (auto *dvi = dyn_cast<DestroyValueInst>(op->getUser())) {
309-
dvi->eraseFromParent();
310-
cvi->eraseFromParent();
408+
eraseInstruction(dvi);
409+
eraseInstructionAndAddOptsToWorklist(cvi);
311410
NumEliminatedInsts += 2;
312411
return true;
313412
}
@@ -331,10 +430,10 @@ static bool eliminateDeadLiveRangeCopyValue(CopyValueInst *cvi) {
331430

332431
// Now that we have a truly dead live range copy value, eliminate it!
333432
while (!destroys.empty()) {
334-
destroys.pop_back_val()->eraseFromParent();
433+
eraseInstruction(destroys.pop_back_val());
335434
++NumEliminatedInsts;
336435
}
337-
cvi->eraseFromParent();
436+
eraseInstructionAndAddOptsToWorklist(cvi);
338437
++NumEliminatedInsts;
339438
return true;
340439
}
@@ -513,12 +612,11 @@ bool SemanticARCOptVisitor::visitLoadInst(LoadInst *li) {
513612
while (!destroyValues.empty()) {
514613
auto *dvi = destroyValues.pop_back_val();
515614
SILBuilderWithScope(dvi).createEndBorrow(dvi->getLoc(), lbi);
516-
dvi->eraseFromParent();
615+
eraseInstruction(dvi);
517616
++NumEliminatedInsts;
518617
}
519618

520-
li->replaceAllUsesWith(lbi);
521-
li->eraseFromParent();
619+
eraseAndRAUWSingleValueInstruction(li, lbi);
522620
++NumEliminatedInsts;
523621
++NumLoadCopyConvertedToLoadBorrow;
524622
return true;
@@ -542,10 +640,11 @@ struct SemanticARCOpts : SILFunctionTransform {
542640
"Can not perform semantic arc optimization unless ownership "
543641
"verification is enabled");
544642

545-
// Iterate over all of the arguments, performing small peephole
546-
// ARC optimizations.
547-
//
548-
// FIXME: Should we iterate or use a RPOT order here?
643+
// Iterate over all of the arguments, performing small peephole ARC
644+
// optimizations. We assume that the visitor will add any instructions we
645+
// need to recursively to the visitor's worklist. Also, note that we assume
646+
// that we do not look through /any/ sil block arguments here since our
647+
// iteration here is only valid up to SSA.
549648
bool madeChange = false;
550649

551650
SemanticARCOptVisitor visitor(f);
@@ -584,6 +683,15 @@ struct SemanticARCOpts : SILFunctionTransform {
584683
madeChange |= visitor.visit(&*ii);
585684
}
586685

686+
// Finally drain the worklist on the visitor and process until we reach the
687+
// fixpoint and thus do not have any further work to do.
688+
//
689+
// NOTE: At this point madeChange has already been set to true if we have
690+
// anything in the worklist, so technically we do not need to do this. But I
691+
// would rather represent this state to future proof the pass to be
692+
// "visually" correct.
693+
madeChange |= visitor.processWorklist();
694+
587695
if (madeChange) {
588696
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
589697
}

test/SILOptimizer/semantic-arc-opts.sil

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -679,3 +679,26 @@ bb0(%x : @owned $ClassLet):
679679

680680
return undef : $()
681681
}
682+
683+
// Make sure that we properly eliminate all ref count ops except for the destroy
684+
// for the @owned argument. The recursion happens since we can not eliminate the
685+
// begin_borrow without eliminating the struct_extract (which we do after we
686+
// eliminate the destroy_value).
687+
// CHECK-LABEL: sil [ossa] @worklist_test : $@convention(thin) (@owned NativeObjectPair) -> () {
688+
// CHECK-NOT: struct_extract
689+
// CHECK: } // end sil function 'worklist_test'
690+
sil [ossa] @worklist_test : $@convention(thin) (@owned NativeObjectPair) -> () {
691+
bb0(%0 : @owned $NativeObjectPair):
692+
%1 = begin_borrow %0 : $NativeObjectPair
693+
%2 = struct_extract %1 : $NativeObjectPair, #NativeObjectPair.obj1
694+
%3 = copy_value %2 : $Builtin.NativeObject
695+
br bb1
696+
697+
bb1:
698+
destroy_value %3 : $Builtin.NativeObject
699+
end_borrow %1 : $NativeObjectPair
700+
destroy_value %0 : $NativeObjectPair
701+
%9999 = tuple()
702+
return %9999 : $()
703+
}
704+

0 commit comments

Comments
 (0)