Skip to content

Commit 2f6ffae

Browse files
gottesmmgottesmm
committed
[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks.
This allows for me to do a couple of things improving quality/correctness/ease of use: 1. I reimplemented InstMod's RAUW and RAUW/erase helpers on top of setUseValue/deleteInst. Beyond allowing the caller to specify less things, we gain an orthogonality preventing bugs like overriding erase/RAUW but not overriding erase or having the erase used in erase/RAUW act differently than the erase for deleteInst. 2. There were a bunch of places using InstModCallback that also were setting uses without having the ability for InstModCallbacks perform it (since it only supported RAUW). This is an anti-pattern and could cause subtle bugs to be introduced by appropriate state in the caller not being updated.
1 parent 126f1ac commit 2f6ffae

File tree

7 files changed

+56
-57
lines changed

7 files changed

+56
-57
lines changed

Diff for: ‎include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ struct CanonicalizeInstruction {
4949
callbacks(
5050
[&](SILInstruction *toDelete) { killInstruction(toDelete); },
5151
[&](SILInstruction *newInst) { notifyNewInstruction(newInst); },
52-
[&](SILValue oldValue, SILValue newValue) {
53-
oldValue->replaceAllUsesWith(newValue);
52+
[&](Operand *use, SILValue newValue) {
53+
use->set(newValue);
5454
notifyHasNewUsers(newValue);
5555
}) {
5656
#ifndef NDEBUG

Diff for: ‎include/swift/SILOptimizer/Utils/InstOptUtils.h

+26-28
Original file line numberDiff line numberDiff line change
@@ -330,19 +330,10 @@ class InstModCallbacks {
330330
/// Default implementation is a no-op, but we still mark madeChange.
331331
std::function<void(SILInstruction *newlyCreatedInst)> createdNewInstFunc;
332332

333-
/// A function that first replaces all uses of oldValue with uses of newValue.
333+
/// A function sets the value in \p use to be \p newValue.
334334
///
335-
/// Default implementation just calls oldValue->replaceAllUsesWith(newValue);
336-
std::function<void(SILValue oldValue, SILValue newValue)>
337-
replaceValueUsesWithFunc;
338-
339-
/// A function that first replaces all uses of oldValue with uses of newValue
340-
/// and then erases oldValue.
341-
///
342-
/// Default implementation just calls replaceValueUsesWithFunc and then
343-
/// deleteInstFunc.
344-
std::function<void(SingleValueInstruction *oldValue, SILValue newValue)>
345-
eraseAndRAUWSingleValueInstFunc;
335+
/// Default implementation just calls use->set(newValue).
336+
std::function<void(Operand *use, SILValue newValue)> setUseValueFunc;
346337

347338
/// A boolean that tracks if any of our callbacks were ever called.
348339
bool madeChange = false;
@@ -352,19 +343,19 @@ class InstModCallbacks {
352343
: deleteInstFunc(deleteInstFunc) {}
353344

354345
InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
355-
decltype(createdNewInstFunc) createdNewInstFunc,
356-
decltype(replaceValueUsesWithFunc) replaceValueUsesWithFunc)
357-
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc),
358-
replaceValueUsesWithFunc(replaceValueUsesWithFunc) {}
346+
decltype(createdNewInstFunc) createdNewInstFunc)
347+
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc) {
348+
}
349+
350+
InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
351+
decltype(setUseValueFunc) setUseValueFunc)
352+
: deleteInstFunc(deleteInstFunc), setUseValueFunc(setUseValueFunc) {}
359353

360-
InstModCallbacks(
361-
decltype(deleteInstFunc) deleteInstFunc,
362-
decltype(createdNewInstFunc) createdNewInstFunc,
363-
decltype(replaceValueUsesWithFunc) replaceValueUsesWithFunc,
364-
decltype(eraseAndRAUWSingleValueInstFunc) eraseAndRAUWSingleValueInstFunc)
354+
InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
355+
decltype(createdNewInstFunc) createdNewInstFunc,
356+
decltype(setUseValueFunc) setUseValueFunc)
365357
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc),
366-
replaceValueUsesWithFunc(replaceValueUsesWithFunc),
367-
eraseAndRAUWSingleValueInstFunc(eraseAndRAUWSingleValueInstFunc) {}
358+
setUseValueFunc(setUseValueFunc) {}
368359

369360
InstModCallbacks() = default;
370361
~InstModCallbacks() = default;
@@ -384,18 +375,25 @@ class InstModCallbacks {
384375
createdNewInstFunc(newlyCreatedInst);
385376
}
386377

378+
void setUseValue(Operand *use, SILValue newValue) {
379+
madeChange = true;
380+
if (setUseValueFunc)
381+
return setUseValueFunc(use, newValue);
382+
use->set(newValue);
383+
}
384+
387385
void replaceValueUsesWith(SILValue oldValue, SILValue newValue) {
388386
madeChange = true;
389-
if (replaceValueUsesWithFunc)
390-
return replaceValueUsesWithFunc(oldValue, newValue);
391-
oldValue->replaceAllUsesWith(newValue);
387+
388+
while (!oldValue->use_empty()) {
389+
auto *use = *oldValue->use_begin();
390+
setUseValue(use, newValue);
391+
}
392392
}
393393

394394
void eraseAndRAUWSingleValueInst(SingleValueInstruction *oldInst,
395395
SILValue newValue) {
396396
madeChange = true;
397-
if (eraseAndRAUWSingleValueInstFunc)
398-
return eraseAndRAUWSingleValueInstFunc(oldInst, newValue);
399397
replaceValueUsesWith(oldInst, newValue);
400398
deleteInst(oldInst);
401399
}

Diff for: ‎lib/SILOptimizer/Analysis/SimplifyInstruction.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,7 @@ swift::replaceAllUsesAndErase(SingleValueInstruction *svi, SILValue newValue,
752752
callbacks.deleteInst(user);
753753
continue;
754754
}
755-
use->set(newValue);
755+
callbacks.setUseValue(use, newValue);
756756
}
757757

758758
callbacks.deleteInst(svi);

Diff for: ‎lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,9 @@ class MandatoryCombiner final
148148
instructionsPendingDeletion.push_back(instruction);
149149
},
150150
[&](SILInstruction *instruction) { worklist.add(instruction); },
151-
[this](SILValue oldValue, SILValue newValue) {
152-
worklist.replaceValueUsesWith(oldValue, newValue);
151+
[this](Operand *use, SILValue newValue) {
152+
use->set(newValue);
153+
worklist.add(use->getUser());
153154
}),
154155
createdInstructions(createdInstructions),
155156
deadEndBlocks(deadEndBlocks){};

Diff for: ‎lib/SILOptimizer/SILCombiner/SILCombiner.h

+16-9
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ class SILCombiner :
8484
/// Cast optimizer
8585
CastOptimizer CastOpt;
8686

87+
/// Centralized InstModCallback that we use for certain utility methods.
88+
InstModCallbacks instModCallbacks;
89+
8790
public:
8891
SILCombiner(SILOptFunctionBuilder &FuncBuilder, SILBuilder &B,
8992
AliasAnalysis *AA, DominanceAnalysis *DA,
@@ -103,7 +106,18 @@ class SILCombiner :
103106
replaceInstUsesWith(*I, V);
104107
},
105108
/* EraseAction */
106-
[&](SILInstruction *I) { eraseInstFromFunction(*I); }) {}
109+
[&](SILInstruction *I) { eraseInstFromFunction(*I); }),
110+
instModCallbacks(
111+
[&](SILInstruction *instToDelete) {
112+
eraseInstFromFunction(*instToDelete);
113+
},
114+
[&](SILInstruction *newlyCreatedInst) {
115+
Worklist.add(newlyCreatedInst);
116+
},
117+
[&](Operand *use, SILValue newValue) {
118+
use->set(newValue);
119+
Worklist.add(use->getUser());
120+
}) {}
107121

108122
bool runOnFunction(SILFunction &F);
109123

@@ -307,14 +321,7 @@ class SILCombiner :
307321
StringRef FInverseName, StringRef FName);
308322

309323
private:
310-
InstModCallbacks getInstModCallbacks() {
311-
return InstModCallbacks(
312-
[this](SILInstruction *DeadInst) { eraseInstFromFunction(*DeadInst); },
313-
[this](SILInstruction *NewInst) { Worklist.add(NewInst); },
314-
[this](SILValue oldValue, SILValue newValue) {
315-
replaceValueUsesWith(oldValue, newValue);
316-
});
317-
}
324+
InstModCallbacks &getInstModCallbacks() { return instModCallbacks; }
318325

319326
// Build concrete existential information using findInitExistential.
320327
Optional<ConcreteOpenedExistentialInfo>

Diff for: ‎lib/SILOptimizer/SemanticARC/SemanticARCOptVisitor.h

+4-11
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,9 @@ struct LLVM_LIBRARY_VISIBILITY SemanticARCOptVisitor
5353
: ctx(fn, onlyGuaranteedOpts,
5454
InstModCallbacks(
5555
[this](SILInstruction *inst) { eraseInstruction(inst); },
56-
[](SILInstruction *) {}, [](SILValue, SILValue) {},
57-
[this](SingleValueInstruction *i, SILValue value) {
58-
eraseAndRAUWSingleValueInstruction(i, value);
56+
[this](Operand *use, SILValue newValue) {
57+
use->set(newValue);
58+
worklist.insert(newValue);
5959
})) {}
6060

6161
void reset() {
@@ -118,14 +118,7 @@ struct LLVM_LIBRARY_VISIBILITY SemanticARCOptVisitor
118118
drainVisitedSinceLastMutationIntoWorklist();
119119
}
120120

121-
InstModCallbacks getCallbacks() {
122-
return InstModCallbacks(
123-
[this](SILInstruction *inst) { eraseInstruction(inst); },
124-
[](SILInstruction *) {}, [](SILValue, SILValue) {},
125-
[this](SingleValueInstruction *i, SILValue value) {
126-
eraseAndRAUWSingleValueInstruction(i, value);
127-
});
128-
}
121+
InstModCallbacks &getCallbacks() { return ctx.instModCallbacks; }
129122

130123
bool visitSILInstruction(SILInstruction *i) {
131124
assert((isa<OwnershipForwardingTermInst>(i) ||

Diff for: ‎lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ void OwnershipRAUWUtility::eliminateReborrowsOfRecursiveBorrows(
434434

435435
// Then set our borrowing operand to take our innerBorrow instead of value
436436
// (whose lifetime we just ended).
437-
borrowingOperand->set(innerBorrow);
437+
callbacks.setUseValue(*borrowingOperand, innerBorrow);
438438
// Add our outer end borrow as a use point to make sure that we extend our
439439
// base value to this point.
440440
usePoints.push_back(&outerEndBorrow->getAllOperands()[0]);
@@ -500,7 +500,7 @@ void OwnershipRAUWUtility::rewriteReborrows(
500500
callbacks.createdNewInst(innerBorrow);
501501
callbacks.createdNewInst(outerEndBorrow);
502502

503-
reborrow->set(innerBorrow);
503+
callbacks.setUseValue(*reborrow, innerBorrow);
504504

505505
auto *borrowedArg =
506506
const_cast<SILPhiArgument *>(bi->getArgForOperand(reborrow.op));
@@ -570,7 +570,7 @@ SILBasicBlock::iterator OwnershipRAUWUtility::handleUnowned() {
570570
auto *newInst = builder.createUncheckedOwnershipConversion(
571571
ti->getLoc(), use->get(), OwnershipKind::Unowned);
572572
callbacks.createdNewInst(newInst);
573-
use->set(newInst);
573+
callbacks.setUseValue(use, newInst);
574574
}
575575
}
576576
}
@@ -602,7 +602,7 @@ SILBasicBlock::iterator OwnershipRAUWUtility::handleUnowned() {
602602
auto *newInst = builder.createUncheckedOwnershipConversion(
603603
ti->getLoc(), use->get(), OwnershipKind::Unowned);
604604
callbacks.createdNewInst(newInst);
605-
use->set(newInst);
605+
callbacks.setUseValue(use, newInst);
606606
}
607607
}
608608
}

0 commit comments

Comments
 (0)