Skip to content

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 5, 2021
Merged

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f6ffae
[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callb…
gottesmm Jan 4, 2021
4600aa7
[sil-inst-opt] Rename the internal madeChange bool in InstModCallback…
gottesmm Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/swift/SILOptimizer/Utils/CanonicalizeInstruction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ struct CanonicalizeInstruction {
callbacks(
[&](SILInstruction *toDelete) { killInstruction(toDelete); },
[&](SILInstruction *newInst) { notifyNewInstruction(newInst); },
[&](SILValue oldValue, SILValue newValue) {
oldValue->replaceAllUsesWith(newValue);
[&](Operand *use, SILValue newValue) {
use->set(newValue);
notifyHasNewUsers(newValue);
}) {
#ifndef NDEBUG
Expand Down
66 changes: 32 additions & 34 deletions include/swift/SILOptimizer/Utils/InstOptUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,77 +330,75 @@ class InstModCallbacks {
/// Default implementation is a no-op, but we still mark madeChange.
std::function<void(SILInstruction *newlyCreatedInst)> createdNewInstFunc;

/// A function that first replaces all uses of oldValue with uses of newValue.
/// A function sets the value in \p use to be \p newValue.
///
/// Default implementation just calls oldValue->replaceAllUsesWith(newValue);
std::function<void(SILValue oldValue, SILValue newValue)>
replaceValueUsesWithFunc;

/// A function that first replaces all uses of oldValue with uses of newValue
/// and then erases oldValue.
///
/// Default implementation just calls replaceValueUsesWithFunc and then
/// deleteInstFunc.
std::function<void(SingleValueInstruction *oldValue, SILValue newValue)>
eraseAndRAUWSingleValueInstFunc;
/// Default implementation just calls use->set(newValue).
std::function<void(Operand *use, SILValue newValue)> setUseValueFunc;

/// A boolean that tracks if any of our callbacks were ever called.
bool madeChange = false;
bool wereAnyCallbacksInvoked = false;

public:
InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc)
: deleteInstFunc(deleteInstFunc) {}

InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
decltype(createdNewInstFunc) createdNewInstFunc,
decltype(replaceValueUsesWithFunc) replaceValueUsesWithFunc)
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc),
replaceValueUsesWithFunc(replaceValueUsesWithFunc) {}
decltype(createdNewInstFunc) createdNewInstFunc)
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc) {
}

InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
decltype(setUseValueFunc) setUseValueFunc)
: deleteInstFunc(deleteInstFunc), setUseValueFunc(setUseValueFunc) {}

InstModCallbacks(
decltype(deleteInstFunc) deleteInstFunc,
decltype(createdNewInstFunc) createdNewInstFunc,
decltype(replaceValueUsesWithFunc) replaceValueUsesWithFunc,
decltype(eraseAndRAUWSingleValueInstFunc) eraseAndRAUWSingleValueInstFunc)
InstModCallbacks(decltype(deleteInstFunc) deleteInstFunc,
decltype(createdNewInstFunc) createdNewInstFunc,
decltype(setUseValueFunc) setUseValueFunc)
: deleteInstFunc(deleteInstFunc), createdNewInstFunc(createdNewInstFunc),
replaceValueUsesWithFunc(replaceValueUsesWithFunc),
eraseAndRAUWSingleValueInstFunc(eraseAndRAUWSingleValueInstFunc) {}
setUseValueFunc(setUseValueFunc) {}

InstModCallbacks() = default;
~InstModCallbacks() = default;
InstModCallbacks(const InstModCallbacks &) = default;
InstModCallbacks(InstModCallbacks &&) = default;

void deleteInst(SILInstruction *instToDelete) {
madeChange = true;
wereAnyCallbacksInvoked = true;
if (deleteInstFunc)
return deleteInstFunc(instToDelete);
instToDelete->eraseFromParent();
}

void createdNewInst(SILInstruction *newlyCreatedInst) {
madeChange = true;
wereAnyCallbacksInvoked = true;
if (createdNewInstFunc)
createdNewInstFunc(newlyCreatedInst);
}

void setUseValue(Operand *use, SILValue newValue) {
wereAnyCallbacksInvoked = true;
if (setUseValueFunc)
return setUseValueFunc(use, newValue);
use->set(newValue);
}

void replaceValueUsesWith(SILValue oldValue, SILValue newValue) {
madeChange = true;
if (replaceValueUsesWithFunc)
return replaceValueUsesWithFunc(oldValue, newValue);
oldValue->replaceAllUsesWith(newValue);
wereAnyCallbacksInvoked = true;

while (!oldValue->use_empty()) {
auto *use = *oldValue->use_begin();
setUseValue(use, newValue);
}
}

void eraseAndRAUWSingleValueInst(SingleValueInstruction *oldInst,
SILValue newValue) {
madeChange = true;
if (eraseAndRAUWSingleValueInstFunc)
return eraseAndRAUWSingleValueInstFunc(oldInst, newValue);
wereAnyCallbacksInvoked = true;
replaceValueUsesWith(oldInst, newValue);
deleteInst(oldInst);
}

bool getMadeChange() const { return madeChange; }
bool hadCallbackInvocation() const { return wereAnyCallbacksInvoked; }
};

/// Get all consumed arguments of a partial_apply.
Expand Down
2 changes: 1 addition & 1 deletion lib/SILOptimizer/Analysis/SimplifyInstruction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ swift::replaceAllUsesAndErase(SingleValueInstruction *svi, SILValue newValue,
callbacks.deleteInst(user);
continue;
}
use->set(newValue);
callbacks.setUseValue(use, newValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the files where I already knew there were issues with us not having full coverage of IR changes via InstModCallbacks. I went through and fixed it in all of the appropriate places to use callbacks.setUseValue();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The obvious fix here would just have been to call the rauw callback, which we'll still need to do anyway.

}

callbacks.deleteInst(svi);
Expand Down
5 changes: 3 additions & 2 deletions lib/SILOptimizer/Mandatory/MandatoryCombine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,9 @@ class MandatoryCombiner final
instructionsPendingDeletion.push_back(instruction);
},
[&](SILInstruction *instruction) { worklist.add(instruction); },
[this](SILValue oldValue, SILValue newValue) {
worklist.replaceValueUsesWith(oldValue, newValue);
[this](Operand *use, SILValue newValue) {
use->set(newValue);
worklist.add(use->getUser());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be doing a hash lookup on every operand update!

}),
createdInstructions(createdInstructions),
deadEndBlocks(deadEndBlocks){};
Expand Down
25 changes: 16 additions & 9 deletions lib/SILOptimizer/SILCombiner/SILCombiner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ class SILCombiner :
/// Cast optimizer
CastOptimizer CastOpt;

/// Centralized InstModCallback that we use for certain utility methods.
InstModCallbacks instModCallbacks;

public:
SILCombiner(SILOptFunctionBuilder &FuncBuilder, SILBuilder &B,
AliasAnalysis *AA, DominanceAnalysis *DA,
Expand All @@ -103,7 +106,18 @@ class SILCombiner :
replaceInstUsesWith(*I, V);
},
/* EraseAction */
[&](SILInstruction *I) { eraseInstFromFunction(*I); }) {}
[&](SILInstruction *I) { eraseInstFromFunction(*I); }),
instModCallbacks(
[&](SILInstruction *instToDelete) {
eraseInstFromFunction(*instToDelete);
},
[&](SILInstruction *newlyCreatedInst) {
Worklist.add(newlyCreatedInst);
},
[&](Operand *use, SILValue newValue) {
use->set(newValue);
Worklist.add(use->getUser());
}) {}

bool runOnFunction(SILFunction &F);

Expand Down Expand Up @@ -307,14 +321,7 @@ class SILCombiner :
StringRef FInverseName, StringRef FName);

private:
InstModCallbacks getInstModCallbacks() {
return InstModCallbacks(
[this](SILInstruction *DeadInst) { eraseInstFromFunction(*DeadInst); },
[this](SILInstruction *NewInst) { Worklist.add(NewInst); },
[this](SILValue oldValue, SILValue newValue) {
replaceValueUsesWith(oldValue, newValue);
});
}
InstModCallbacks &getInstModCallbacks() { return instModCallbacks; }

// Build concrete existential information using findInitExistential.
Optional<ConcreteOpenedExistentialInfo>
Expand Down
15 changes: 4 additions & 11 deletions lib/SILOptimizer/SemanticARC/SemanticARCOptVisitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ struct LLVM_LIBRARY_VISIBILITY SemanticARCOptVisitor
: ctx(fn, onlyGuaranteedOpts,
InstModCallbacks(
[this](SILInstruction *inst) { eraseInstruction(inst); },
[](SILInstruction *) {}, [](SILValue, SILValue) {},
[this](SingleValueInstruction *i, SILValue value) {
eraseAndRAUWSingleValueInstruction(i, value);
[this](Operand *use, SILValue newValue) {
use->set(newValue);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hunk may be slightly confusing. What is happening is that rather than passing an empty argument for createdNewInst, I am just using an overload of InstModCallback's constructor that omits it (and thus assumes we want the default... which we do).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single RAUW could replace thousands of operands. We design IR's very carefully so operand setting can be as efficient as possible. We definitely don't want to do a hash lookup each time. There only needs to be a single worklist insertion per RAUW operation.

worklist.insert(newValue);
})) {}

void reset() {
Expand Down Expand Up @@ -118,14 +118,7 @@ struct LLVM_LIBRARY_VISIBILITY SemanticARCOptVisitor
drainVisitedSinceLastMutationIntoWorklist();
}

InstModCallbacks getCallbacks() {
return InstModCallbacks(
[this](SILInstruction *inst) { eraseInstruction(inst); },
[](SILInstruction *) {}, [](SILValue, SILValue) {},
[this](SingleValueInstruction *i, SILValue value) {
eraseAndRAUWSingleValueInstruction(i, value);
});
}
InstModCallbacks &getCallbacks() { return ctx.instModCallbacks; }

bool visitSILInstruction(SILInstruction *i) {
assert((isa<OwnershipForwardingTermInst>(i) ||
Expand Down
8 changes: 4 additions & 4 deletions lib/SILOptimizer/Utils/OwnershipOptUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,7 @@ void OwnershipRAUWUtility::eliminateReborrowsOfRecursiveBorrows(

// Then set our borrowing operand to take our innerBorrow instead of value
// (whose lifetime we just ended).
borrowingOperand->set(innerBorrow);
callbacks.setUseValue(*borrowingOperand, innerBorrow);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the files where I already knew there were issues with us not having full coverage of IR changes via InstModCallbacks. I went through and fixed it in all of the appropriate places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but who needs to know that borrowingOperand changed as opposed to innerBorrow?

// Add our outer end borrow as a use point to make sure that we extend our
// base value to this point.
usePoints.push_back(&outerEndBorrow->getAllOperands()[0]);
Expand Down Expand Up @@ -500,7 +500,7 @@ void OwnershipRAUWUtility::rewriteReborrows(
callbacks.createdNewInst(innerBorrow);
callbacks.createdNewInst(outerEndBorrow);

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

auto *borrowedArg =
const_cast<SILPhiArgument *>(bi->getArgForOperand(reborrow.op));
Expand Down Expand Up @@ -570,7 +570,7 @@ SILBasicBlock::iterator OwnershipRAUWUtility::handleUnowned() {
auto *newInst = builder.createUncheckedOwnershipConversion(
ti->getLoc(), use->get(), OwnershipKind::Unowned);
callbacks.createdNewInst(newInst);
use->set(newInst);
callbacks.setUseValue(use, newInst);
}
}
}
Expand Down Expand Up @@ -602,7 +602,7 @@ SILBasicBlock::iterator OwnershipRAUWUtility::handleUnowned() {
auto *newInst = builder.createUncheckedOwnershipConversion(
ti->getLoc(), use->get(), OwnershipKind::Unowned);
callbacks.createdNewInst(newInst);
use->set(newInst);
callbacks.setUseValue(use, newInst);
}
}
}
Expand Down