Skip to content

Commit ddd0f4d

Browse files
eecksteineeckstein
committed
SIL: replace SmallPtrSet<SILBasicBlock *> with BasicBlockSet in LinearLifetimeChecker
This makes it unnecessary to share a single instance of visited blocks across multiple calls the the checker.
1 parent c638a2f commit ddd0f4d

13 files changed

+46
-80
lines changed

‎include/swift/SIL/LinearLifetimeChecker.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
#include "swift/SIL/SILBasicBlock.h"
2121
#include "swift/SIL/SILFunction.h"
2222
#include "swift/SIL/SILValue.h"
23+
#include "swift/SIL/BasicBlockUtils.h"
24+
#include "swift/SIL/SILBitfield.h"
2325
#include "llvm/ADT/SmallPtrSet.h"
2426

2527
namespace swift {
@@ -28,7 +30,6 @@ class SILBasicBlock;
2830
class SILInstruction;
2931
class SILModule;
3032
class SILValue;
31-
class DeadEndBlocks;
3233
class SILOwnershipVerifier;
3334
class SILValueOwnershipChecker;
3435

@@ -56,13 +57,11 @@ class LinearLifetimeChecker {
5657
friend class SILOwnershipVerifier;
5758
friend class SILValueOwnershipChecker;
5859

59-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks;
6060
DeadEndBlocks &deadEndBlocks;
6161

6262
public:
63-
LinearLifetimeChecker(SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
64-
DeadEndBlocks &deadEndBlocks)
65-
: visitedBlocks(visitedBlocks), deadEndBlocks(deadEndBlocks) {}
63+
LinearLifetimeChecker(DeadEndBlocks &deadEndBlocks)
64+
: deadEndBlocks(deadEndBlocks) {}
6665

6766
/// Returns true that \p value forms a linear lifetime with consuming uses \p
6867
/// consumingUses, non consuming uses \p nonConsumingUses. Returns false

‎include/swift/SIL/OwnershipUtils.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,6 @@ struct BorrowedValue {
448448
/// borrow scopes if needed.
449449
bool areUsesWithinScope(ArrayRef<Operand *> uses,
450450
SmallVectorImpl<Operand *> &scratchSpace,
451-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
452451
DeadEndBlocks &deadEndBlocks) const;
453452

454453
/// Given a local borrow scope introducer, visit all non-forwarding consuming

‎lib/SIL/Utils/OwnershipUtils.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -404,12 +404,10 @@ llvm::raw_ostream &swift::operator<<(llvm::raw_ostream &os,
404404

405405
bool BorrowedValue::areUsesWithinScope(
406406
ArrayRef<Operand *> uses, SmallVectorImpl<Operand *> &scratchSpace,
407-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
408407
DeadEndBlocks &deadEndBlocks) const {
409408
// Make sure that we clear our scratch space/utilities before we exit.
410409
SWIFT_DEFER {
411410
scratchSpace.clear();
412-
visitedBlocks.clear();
413411
};
414412

415413
// First make sure that we actually have a local scope. If we have a non-local
@@ -425,7 +423,7 @@ bool BorrowedValue::areUsesWithinScope(
425423
visitLocalScopeTransitiveEndingUses(
426424
[&scratchSpace](Operand *op) { scratchSpace.emplace_back(op); });
427425

428-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
426+
LinearLifetimeChecker checker(deadEndBlocks);
429427
return checker.validateLifetime(value, scratchSpace, uses);
430428
}
431429

‎lib/SIL/Verifier/LinearLifetimeChecker.cpp

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ struct State {
6767
LinearLifetimeChecker::ErrorBuilder &errorBuilder;
6868

6969
/// The blocks that we have already visited.
70-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks;
70+
BasicBlockSet visitedBlocks;
7171

7272
/// If non-null a callback that we should pass any detected leaking blocks for
7373
/// our caller. The intention is that this can be used in a failing case to
@@ -86,7 +86,7 @@ struct State {
8686
ArrayRef<Operand *> nonConsumingUses;
8787

8888
/// The set of blocks with consuming uses.
89-
SmallPtrSet<SILBasicBlock *, 8> blocksWithConsumingUses;
89+
BasicBlockSet blocksWithConsumingUses;
9090

9191
/// The set of blocks with non-consuming uses and the associated
9292
/// non-consuming use SILInstruction.
@@ -105,32 +105,33 @@ struct State {
105105
/// terminates.
106106
SmallSetVector<SILBasicBlock *, 8> successorBlocksThatMustBeVisited;
107107

108-
State(SILValue value, SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
108+
State(SILValue value,
109109
LinearLifetimeChecker::ErrorBuilder &errorBuilder,
110110
Optional<function_ref<void(SILBasicBlock *)>> leakingBlockCallback,
111111
Optional<function_ref<void(Operand *)>>
112112
nonConsumingUseOutsideLifetimeCallback,
113113
ArrayRef<Operand *> consumingUses, ArrayRef<Operand *> nonConsumingUses)
114114
: value(value), beginInst(value->getDefiningInsertionPoint()),
115-
errorBuilder(errorBuilder), visitedBlocks(visitedBlocks),
115+
errorBuilder(errorBuilder), visitedBlocks(value->getFunction()),
116116
leakingBlockCallback(leakingBlockCallback),
117117
nonConsumingUseOutsideLifetimeCallback(
118118
nonConsumingUseOutsideLifetimeCallback),
119-
consumingUses(consumingUses), nonConsumingUses(nonConsumingUses) {}
119+
consumingUses(consumingUses), nonConsumingUses(nonConsumingUses),
120+
blocksWithConsumingUses(value->getFunction()) {}
120121

121122
State(SILBasicBlock *beginBlock,
122-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
123123
LinearLifetimeChecker::ErrorBuilder &errorBuilder,
124124
Optional<function_ref<void(SILBasicBlock *)>> leakingBlockCallback,
125125
Optional<function_ref<void(Operand *)>>
126126
nonConsumingUseOutsideLifetimeCallback,
127127
ArrayRef<Operand *> consumingUses, ArrayRef<Operand *> nonConsumingUses)
128128
: value(), beginInst(&*beginBlock->begin()), errorBuilder(errorBuilder),
129-
visitedBlocks(visitedBlocks),
129+
visitedBlocks(beginBlock->getParent()),
130130
leakingBlockCallback(leakingBlockCallback),
131131
nonConsumingUseOutsideLifetimeCallback(
132132
nonConsumingUseOutsideLifetimeCallback),
133-
consumingUses(consumingUses), nonConsumingUses(nonConsumingUses) {}
133+
consumingUses(consumingUses), nonConsumingUses(nonConsumingUses),
134+
blocksWithConsumingUses(beginBlock->getParent()) {}
134135

135136
SILBasicBlock *getBeginBlock() const { return beginInst->getParent(); }
136137

@@ -273,7 +274,7 @@ void State::initializeConsumingUse(Operand *consumingUse,
273274
SILBasicBlock *userBlock) {
274275
// Map this user to the block. If we already have a value for the block, then
275276
// we have a double consume and need to fail.
276-
if (blocksWithConsumingUses.insert(userBlock).second)
277+
if (blocksWithConsumingUses.insert(userBlock))
277278
return;
278279

279280
errorBuilder.handleOverConsume([&] {
@@ -345,15 +346,15 @@ void State::checkForSameBlockUseAfterFree(Operand *consumingUse,
345346

346347
void State::checkPredsForDoubleConsume(Operand *consumingUse,
347348
SILBasicBlock *userBlock) {
348-
if (!blocksWithConsumingUses.count(userBlock))
349+
if (!blocksWithConsumingUses.contains(userBlock))
349350
return;
350351

351352
// Check if this is a block that we have already visited. This means that we
352353
// had a back edge of some sort. Double check that we haven't missed any
353354
// successors.
354-
if (visitedBlocks.count(userBlock)) {
355+
if (visitedBlocks.contains(userBlock)) {
355356
for (auto *succ : userBlock->getSuccessorBlocks()) {
356-
if (!visitedBlocks.count(succ)) {
357+
if (!visitedBlocks.contains(succ)) {
357358
successorBlocksThatMustBeVisited.insert(succ);
358359
}
359360
}
@@ -375,15 +376,15 @@ void State::checkPredsForDoubleConsume(Operand *consumingUse,
375376
}
376377

377378
void State::checkPredsForDoubleConsume(SILBasicBlock *userBlock) {
378-
if (!blocksWithConsumingUses.count(userBlock))
379+
if (!blocksWithConsumingUses.contains(userBlock))
379380
return;
380381

381382
// Check if this is a block that we have already visited. This means that we
382383
// had a back edge of some sort. Double check that we haven't missed any
383384
// successors.
384-
if (visitedBlocks.count(userBlock)) {
385+
if (visitedBlocks.contains(userBlock)) {
385386
for (auto *succ : userBlock->getSuccessorBlocks()) {
386-
if (!visitedBlocks.count(succ)) {
387+
if (!visitedBlocks.contains(succ)) {
387388
successorBlocksThatMustBeVisited.insert(succ);
388389
}
389390
}
@@ -438,7 +439,7 @@ void State::performDataflow(DeadEndBlocks &deBlocks) {
438439
for (auto *succBlock : block->getSuccessorBlocks()) {
439440
// If we already visited the successor, there is nothing to do since we
440441
// already visited the successor.
441-
if (visitedBlocks.count(succBlock))
442+
if (visitedBlocks.contains(succBlock))
442443
continue;
443444

444445
// Then check if the successor is a transitively unreachable block. In
@@ -470,7 +471,7 @@ void State::performDataflow(DeadEndBlocks &deBlocks) {
470471
// Check if we have an over consume.
471472
checkPredsForDoubleConsume(predBlock);
472473

473-
if (visitedBlocks.count(predBlock)) {
474+
if (visitedBlocks.contains(predBlock)) {
474475
continue;
475476
}
476477

@@ -563,7 +564,7 @@ LinearLifetimeChecker::Error LinearLifetimeChecker::checkValueImpl(
563564
// || deadEndBlocks.isDeadEnd(value->getParentBlock())) &&
564565
// "Must have at least one consuming user?!");
565566

566-
State state(value, visitedBlocks, errorBuilder, leakingBlockCallback,
567+
State state(value, errorBuilder, leakingBlockCallback,
567568
nonConsumingUseOutsideLifetimeCallback, consumingUses,
568569
nonConsumingUses);
569570

@@ -637,7 +638,7 @@ LinearLifetimeChecker::Error LinearLifetimeChecker::checkValueImpl(
637638
// list.
638639
state.checkPredsForDoubleConsume(use, predBlock);
639640

640-
if (!state.visitedBlocks.insert(predBlock).second)
641+
if (!state.visitedBlocks.insert(predBlock))
641642
continue;
642643

643644
state.worklist.push_back(predBlock);

‎lib/SIL/Verifier/LoadBorrowImmutabilityChecker.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,7 @@ bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
291291
LoadBorrowInst *lbi, ArrayRef<Operand *> endBorrowUses,
292292
AccessPath accessPath) {
293293

294-
SmallPtrSet<SILBasicBlock *, 8> visitedBlocks;
295-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
294+
LinearLifetimeChecker checker(deadEndBlocks);
296295
auto writes = cache.get(accessPath);
297296

298297
// Treat None as a write.
@@ -303,8 +302,6 @@ bool LoadBorrowImmutabilityAnalysis::isImmutableInScope(
303302
}
304303
// Then for each write...
305304
for (auto *op : *writes) {
306-
visitedBlocks.clear();
307-
308305
// First see if the write is a dead end block. In such a case, just skip it.
309306
if (deadEndBlocks.isDeadEnd(op->getUser()->getParent())) {
310307
continue;

‎lib/SIL/Verifier/ReborrowVerifier.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,12 @@ using namespace swift;
1818

1919
bool ReborrowVerifier::verifyReborrowLifetime(SILPhiArgument *phiArg,
2020
SILValue baseVal) {
21-
SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;
2221
bool result = false;
2322
SmallVector<Operand *, 4> phiArgUses(phiArg->getUses());
2423

2524
// Verify whether the guaranteed phi arg lies within the lifetime of the base
2625
// value.
27-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
26+
LinearLifetimeChecker checker(deadEndBlocks);
2827
// newErrorBuilder is consumed at the end of the checkValue function.
2928
// Copy initial state from errorBuilder everytime
3029
LinearLifetimeChecker::ErrorBuilder newErrorBuilder = errorBuilder;

‎lib/SIL/Verifier/SILOwnershipVerifier.cpp

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -104,19 +104,15 @@ class SILValueOwnershipChecker {
104104
/// is successful.
105105
SmallVector<Operand *, 16> regularUsers;
106106

107-
/// The set of blocks that we have visited.
108-
SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks;
109-
110107
ReborrowVerifier &reborrowVerifier;
111108

112109
public:
113110
SILValueOwnershipChecker(
114111
DeadEndBlocks &deadEndBlocks, SILValue value,
115112
LinearLifetimeChecker::ErrorBuilder &errorBuilder,
116-
llvm::SmallPtrSetImpl<SILBasicBlock *> &visitedBlocks,
117113
ReborrowVerifier &reborrowVerifier)
118114
: result(), deadEndBlocks(deadEndBlocks), value(value),
119-
errorBuilder(errorBuilder), visitedBlocks(visitedBlocks),
115+
errorBuilder(errorBuilder),
120116
reborrowVerifier(reborrowVerifier) {
121117
assert(value && "Can not initialize a checker with an empty SILValue");
122118
}
@@ -170,7 +166,7 @@ bool SILValueOwnershipChecker::check() {
170166
SmallVector<Operand *, 32> allRegularUsers;
171167
llvm::copy(regularUsers, std::back_inserter(allRegularUsers));
172168

173-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
169+
LinearLifetimeChecker checker(deadEndBlocks);
174170
auto linearLifetimeResult = checker.checkValue(value, allLifetimeEndingUsers,
175171
allRegularUsers, errorBuilder);
176172
result = !linearLifetimeResult.getFoundError();
@@ -523,8 +519,7 @@ bool SILValueOwnershipChecker::checkYieldWithoutLifetimeEndingUses(
523519
coroutineEndUses.push_back(use);
524520
}
525521

526-
assert(visitedBlocks.empty());
527-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
522+
LinearLifetimeChecker checker(deadEndBlocks);
528523
auto linearLifetimeResult =
529524
checker.checkValue(yield, coroutineEndUses, regularUses, errorBuilder);
530525
if (linearLifetimeResult.getFoundError()) {
@@ -771,8 +766,7 @@ verifySILValueHelper(const SILFunction *f, SILValue value,
771766
if (!f->hasOwnership() || !f->shouldVerifyOwnership())
772767
return;
773768

774-
SmallPtrSet<SILBasicBlock *, 32> liveBlocks;
775-
SILValueOwnershipChecker(*deadEndBlocks, value, errorBuilder, liveBlocks,
769+
SILValueOwnershipChecker(*deadEndBlocks, value, errorBuilder,
776770
reborrowVerifier)
777771
.check();
778772
}

‎lib/SILOptimizer/Mandatory/MandatoryInlining.cpp

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ static bool fixupReferenceCounts(
7171
// can use this to copy if we need to.
7272
assert(captureArgConventions.size() == capturedArgs.size());
7373

74-
SmallPtrSet<SILBasicBlock *, 8> visitedBlocks;
7574
// FIXME: Can we cache this in between inlining invocations?
7675
DeadEndBlocks deadEndBlocks(pai->getFunction());
7776
SmallVector<SILBasicBlock *, 4> leakingBlocks;
@@ -111,9 +110,8 @@ static bool fixupReferenceCounts(
111110
SILBuilderWithScope builder(pai);
112111
auto *stackLoc = builder.createAllocStack(loc, v->getType().getObjectType());
113112
builder.createCopyAddr(loc, v, stackLoc, IsNotTake, IsInitialization);
114-
visitedBlocks.clear();
115113

116-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
114+
LinearLifetimeChecker checker(deadEndBlocks);
117115
bool consumedInLoop = checker.completeConsumingUseSet(
118116
pai, applySite.getCalleeOperand(),
119117
[&](SILBasicBlock::iterator insertPt) {
@@ -144,8 +142,6 @@ static bool fixupReferenceCounts(
144142
argument = SILBuilderWithScope(pai).createBeginBorrow(loc, argument);
145143
}
146144

147-
visitedBlocks.clear();
148-
149145
// If we need to insert compensating destroys, do so.
150146
//
151147
// NOTE: We use pai here since in non-ossa code emitCopyValueOperation
@@ -158,7 +154,7 @@ static bool fixupReferenceCounts(
158154
// just cares about the block the value is in. In a forthcoming commit, I
159155
// am going to change this to use a different API on the linear lifetime
160156
// checker that makes this clearer.
161-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
157+
LinearLifetimeChecker checker(deadEndBlocks);
162158
bool consumedInLoop = checker.completeConsumingUseSet(
163159
pai, applySite.getCalleeOperand(),
164160
[&](SILBasicBlock::iterator insertPt) {
@@ -190,7 +186,6 @@ static bool fixupReferenceCounts(
190186
// TODO: Do we need to lifetime extend here?
191187
case ParameterConvention::Direct_Unowned: {
192188
v = SILBuilderWithScope(pai).emitCopyValueOperation(loc, v);
193-
visitedBlocks.clear();
194189

195190
// If our consuming partial apply does not post-dominate our
196191
// partial_apply, compute the completion of the post dominance set and if
@@ -206,7 +201,7 @@ static bool fixupReferenceCounts(
206201
// just cares about the block the value is in. In a forthcoming commit, I
207202
// am going to change this to use a different API on the linear lifetime
208203
// checker that makes this clearer.
209-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
204+
LinearLifetimeChecker checker(deadEndBlocks);
210205
checker.completeConsumingUseSet(
211206
pai, applySite.getCalleeOperand(),
212207
[&](SILBasicBlock::iterator insertPt) {
@@ -230,7 +225,6 @@ static bool fixupReferenceCounts(
230225
// apply has another use that would destroy our value first.
231226
case ParameterConvention::Direct_Owned: {
232227
v = SILBuilderWithScope(pai).emitCopyValueOperation(loc, v);
233-
visitedBlocks.clear();
234228

235229
// If we need to insert compensating destroys, do so.
236230
//
@@ -244,7 +238,7 @@ static bool fixupReferenceCounts(
244238
// just cares about the block the value is in. In a forthcoming commit, I
245239
// am going to change this to use a different API on the linear lifetime
246240
// checker that makes this clearer.
247-
LinearLifetimeChecker checker(visitedBlocks, deadEndBlocks);
241+
LinearLifetimeChecker checker(deadEndBlocks);
248242
checker.completeConsumingUseSet(
249243
pai, applySite.getCalleeOperand(),
250244
[&](SILBasicBlock::iterator insertPt) {

0 commit comments

Comments
 (0)