Skip to content

[semantic-arc-opts] Eliminate all copy_value from guaranteed argument… #19690

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
Nov 13, 2018
Merged

[semantic-arc-opts] Eliminate all copy_value from guaranteed argument… #19690

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
5a73063
[semantic-arc-opts] Eliminate all copy_value from guaranteed argument…
gottesmm Oct 2, 2018
099cae5
[semantic-arc-opts] Handle arg extract + copy + borrow + use.
gottesmm Oct 3, 2018
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
5 changes: 4 additions & 1 deletion include/swift/SIL/OwnershipUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,12 @@ bool isGuaranteedForwardingValueKind(SILNodeKind kind);

bool isGuaranteedForwardingValue(SILValue value);

bool isOwnershipForwardingInst(SILInstruction *i);

bool isGuaranteedForwardingInst(SILInstruction *i);

bool isOwnershipForwardingInst(SILInstruction *i);
bool getUnderlyingBorrowIntroducers(SILValue inputValue,
SmallVectorImpl<SILValue> &out);

} // namespace swift

Expand Down
36 changes: 36 additions & 0 deletions lib/SIL/OwnershipUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//

#include "swift/SIL/OwnershipUtils.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILInstruction.h"

using namespace swift;
Expand Down Expand Up @@ -75,3 +76,38 @@ bool swift::isGuaranteedForwardingInst(SILInstruction *i) {
bool swift::isOwnershipForwardingInst(SILInstruction *i) {
return isOwnershipForwardingValueKind(SILNodeKind(i->getKind()));
}

bool swift::getUnderlyingBorrowIntroducers(SILValue inputValue,
SmallVectorImpl<SILValue> &out) {
SmallVector<SILValue, 32> worklist;
worklist.emplace_back(inputValue);

while (!worklist.empty()) {
SILValue v = worklist.pop_back_val();

// First check if v is an introducer. If so, stash it and continue.
if (isa<SILFunctionArgument>(v) || isa<LoadBorrowInst>(v) ||
isa<BeginBorrowInst>(v)) {
out.push_back(v);
continue;
}

// Otherwise if v is an ownership forwarding value, add its defining
// instruction
if (isGuaranteedForwardingValue(v)) {
auto *i = v->getDefiningInstruction();
assert(i);
transform(i->getAllOperands(), std::back_inserter(worklist),
[](const Operand &op) -> SILValue { return op.get(); });
continue;
}

// If v produces any ownership, then we can ignore it. Otherwise, we need to
// return false since this is an introducer we do not understand.
if (v.getOwnershipKind() != ValueOwnershipKind::Any &&
v.getOwnershipKind() != ValueOwnershipKind::Trivial)
return false;
}

return true;
}
284 changes: 166 additions & 118 deletions lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
//===----------------------------------------------------------------------===//

#define DEBUG_TYPE "sil-semantic-arc-opts"
#include "swift/Basic/STLExtras.h"
#include "swift/SIL/BasicBlockUtils.h"
#include "swift/SIL/OwnershipUtils.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILInstruction.h"
#include "swift/SIL/SILVisitor.h"
#include "swift/SILOptimizer/Analysis/PostOrderAnalysis.h"
#include "swift/SILOptimizer/PassManager/Passes.h"
#include "swift/SILOptimizer/PassManager/Transforms.h"
Expand All @@ -26,123 +28,144 @@ using namespace swift;

STATISTIC(NumEliminatedInsts, "number of removed instructions");

static bool optimizeGuaranteedArgument(SILArgument *Arg,
OwnershipChecker &Checker) {
bool MadeChange = false;
namespace {

// Gather all copy_value users of Arg.
llvm::SmallVector<CopyValueInst *, 4> Worklist;
for (auto *Op : Arg->getUses()) {
if (auto *CVI = dyn_cast<CopyValueInst>(Op->getUser())) {
Worklist.push_back(CVI);
}
}
struct SemanticARCOptVisitor
: SILInstructionVisitor<SemanticARCOptVisitor, bool> {
bool visitSILInstruction(SILInstruction *i) { return false; }
bool visitCopyValueInst(CopyValueInst *cvi);
bool visitBeginBorrowInst(BeginBorrowInst *bbi);
};

} // end anonymous namespace

// Then until we run out of copies...
while (!Worklist.empty()) {
auto *CVI = Worklist.pop_back_val();

// Quickly see if copy has only one use and that use is a destroy_value. In
// such a case, we can always eliminate both the copy and the destroy.
if (auto *Op = CVI->getSingleUse()) {
if (auto *DVI = dyn_cast<DestroyValueInst>(Op->getUser())) {
DVI->eraseFromParent();
CVI->eraseFromParent();
NumEliminatedInsts += 2;
bool SemanticARCOptVisitor::visitBeginBorrowInst(BeginBorrowInst *bbi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] I thought we were using all-caps for acronyms: BBI? I don't actually care, but if not, there will be quite a lot of code to rewrite.

auto kind = bbi->getOperand().getOwnershipKind();
SmallVector<EndBorrowInst *, 16> endBorrows;
for (auto *op : bbi->getUses()) {
auto *user = op->getUser();
switch (user->getKind()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I don't understand why a switch statement is used here.

case SILInstructionKind::EndBorrowInst:
endBorrows.push_back(cast<EndBorrowInst>(user));
break;
default:
// Make sure that this operand can accept our arguments kind.
auto map = op->getOwnershipKindMap();
if (map.canAcceptKind(kind))
Copy link
Contributor

Choose a reason for hiding this comment

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

[design] I still don't understand what an OwnershipKindMap is for. I think it obscures the meaning of the code. Why not just:

op->canAcceptOwnership(kind)

Also, this check doesn't make sense to me for forwarding instructions, without teaching the optimization to look through all of those. If the forwarding instruction is not taking ownership before, we don't want to implicitly change it's meaning by changing the ownership of its incoming value.

This is what would make sense to me, instead of the map and "can accept" check.

// Is this a point-in-time use that cannot extend the lifetime if it's
// operand value?
if (op->isNonConsuming())
  continue;

return false

continue;
}
return false;
}
}

// Ok, now run the checker on the copy value. If it fails, then we just
// continue.
if (!Checker.checkValue(CVI))
continue;

// Otherwise, lets do a quick check on what the checker thinks the lifetime
// ending and non-lifetime ending users. To be conservative, we bail unless
// each lifetime ending use is a destroy_value and if each non-lifetime
// ending use is one of the following instructions:
//
// 1. copy_value.
// 2. begin_borrow.
// 3. end_borrow.
if (!all_of(Checker.lifetimeEndingUsers, [](SILInstruction *I) -> bool {
return isa<DestroyValueInst>(I);
}))
continue;

// Extra copy values that we should visit recursively.
llvm::SmallVector<CopyValueInst *, 8> NewCopyInsts;
llvm::SmallVector<SILInstruction *, 8> NewBorrowInsts;
if (!all_of(Checker.regularUsers, [&](SILInstruction *I) -> bool {
if (auto *CVI = dyn_cast<CopyValueInst>(I)) {
NewCopyInsts.push_back(CVI);
return true;
}

if (!isa<BeginBorrowInst>(I) && !isa<EndBorrowInst>(I))
return false;

NewBorrowInsts.push_back(I);
return true;
}))
continue;

// Ok! we can remove the copy_value, destroy_values!
MadeChange = true;
CVI->replaceAllUsesWith(CVI->getOperand());
CVI->eraseFromParent();
// At this point, we know that the begin_borrow's operand can be
// used as an argument to all non-end borrow uses. Eliminate the
// begin borrow and end borrows.
while (!endBorrows.empty()) {
auto *ebi = endBorrows.pop_back_val();
ebi->eraseFromParent();
++NumEliminatedInsts;
}
bbi->replaceAllUsesWith(bbi->getOperand());
bbi->eraseFromParent();
++NumEliminatedInsts;
return true;
}

while (!Checker.lifetimeEndingUsers.empty()) {
Checker.lifetimeEndingUsers.pop_back_val()->eraseFromParent();
++NumEliminatedInsts;
}

// Then add the copy_values that were users of our original copy value to
// the worklist.
while (!NewCopyInsts.empty()) {
Worklist.push_back(NewCopyInsts.pop_back_val());
}

// Then remove any begin/end borrow that we found. These are unneeded since
// the lifetime guarantee from the argument exists above and beyond said
// scope.
while (!NewBorrowInsts.empty()) {
SILInstruction *I = NewBorrowInsts.pop_back_val();
if (auto *BBI = dyn_cast<BeginBorrowInst>(I)) {
// Any copy_value that is used by the begin borrow is added to the
// worklist.
for (auto *BBIUse : BBI->getUses()) {
if (auto *BBIUseCopyValue =
dyn_cast<CopyValueInst>(BBIUse->getUser())) {
Worklist.push_back(BBIUseCopyValue);
}
}
static bool canHandleOperand(SILValue operand, SmallVectorImpl<SILValue> &out) {
if (!getUnderlyingBorrowIntroducers(operand, out))
return false;

// First go through and eliminate all end borrows.
SmallVector<EndBorrowInst *, 4> endBorrows;
copy(BBI->getEndBorrows(), std::back_inserter(endBorrows));
while (!endBorrows.empty()) {
endBorrows.pop_back_val()->eraseFromParent();
++NumEliminatedInsts;
}
/// TODO: Add support for begin_borrow, load_borrow.
return all_of(out, [](SILValue v) { return isa<SILFunctionArgument>(v); });
}

// Then eliminate BBI itself.
BBI->replaceAllUsesWith(BBI->getOperand());
BBI->eraseFromParent();
++NumEliminatedInsts;
static bool performGuaranteedCopyValueOptimization(CopyValueInst *cvi) {
SmallVector<SILValue, 16> borrowIntroducers;

// Whitelist the operands that we know how to support and make sure
// our operand is actually guaranteed.
if (!canHandleOperand(cvi->getOperand(), borrowIntroducers))
return false;

// Then go over all of our uses. Find our destroying instructions
// and make sure all of them are destroy_value. For our
// non-destroying instructions, make sure that they accept a
// guaranteed value. After that, make sure that our destroys are
// within the lifetime of our borrowed values.
SmallVector<DestroyValueInst *, 16> destroys;
for (auto *op : cvi->getUses()) {
// We know that a copy_value produces an @owned value. Look
// through all of our uses and classify them as either
// invalidating or not invalidating. Make sure that all of the
// invalidating ones are destroy_value since otherwise the
// live_range is not complete.
auto map = op->getOwnershipKindMap();
auto constraint = map.getLifetimeConstraint(ValueOwnershipKind::Owned);
switch (constraint) {
case UseLifetimeConstraint::MustBeInvalidated:
// And we have a destroy_value, track it and continue.
if (auto *dvi = dyn_cast<DestroyValueInst>(op->getUser())) {
destroys.push_back(dvi);
continue;
}
// Otherwise, we found a non-destroy value invalidating owned
// user... This is not an unnecessary live range.
return false;
case UseLifetimeConstraint::MustBeLive:
// Ok, this constraint can take something owned as live. Lets
// see if it can also take something that is guaranteed. If it
// can not, then we bail.
if (!map.canAcceptKind(ValueOwnershipKind::Guaranteed)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[design] I really can't mentally map the code that's using these physical maps to concepts that I normally reason about. i.e. If I wanted to know how this would work on a concrete SIL example, I would have to construct a test case by hand and run it through the code. (that's the problem with too much indirection). I think this is what the code is trying to do (eliding some details), but not sure:

switch (operand.ownershipEffect()):
case Consuming:
  if (isa<Destroy>(operand.getUser()))
    continue;
  return false;

case NonConsuming:
  // A point-in-time use can always handle +1 or +0.
  continue;

case Forwarding:
  // This instruction takes ownership of the copy. We don't currently
  // recurse through them.
  return false;
}

If we wanted to constrain some non-consuming users to only handle owned
values, then we could introduce a separate "use constraint" API for
that. But I suspect we don't want to allow that anyway.

return false;
}

// This is not necessary, but it does add a check.
auto *EBI = cast<EndBorrowInst>(I);
EBI->eraseFromParent();
++NumEliminatedInsts;
// Otherwise, continue.
continue;
}
}

return MadeChange;
// If we reached this point, then we know that all of our users can
// accept a guaranteed value and our owned value is destroyed only
// by destroy_value. Check if all of our destroys are joint
// post-dominated by the end_borrow set. If they do not, then the
// copy_value is lifetime extending the guaranteed value, we can not
// eliminate it.
//
// TODO: When we support begin_borrow/load_borrow a linear linfetime
// check will be needed here.
assert(all_of(borrowIntroducers,
[](SILValue v) { return isa<SILFunctionArgument>(v); }));

// Otherwise, we know that our copy_value/destroy_values are all
// completely within the guaranteed value scope.
while (!destroys.empty()) {
auto *dvi = destroys.pop_back_val();
dvi->eraseFromParent();
++NumEliminatedInsts;
}
cvi->replaceAllUsesWith(cvi->getOperand());
cvi->eraseFromParent();
++NumEliminatedInsts;
return true;
}

bool SemanticARCOptVisitor::visitCopyValueInst(CopyValueInst *cvi) {
// If our copy value inst has a single destroy value user, eliminate
// it.
if (auto *op = cvi->getSingleUse()) {
if (auto *dvi = dyn_cast<DestroyValueInst>(op->getUser())) {
dvi->eraseFromParent();
cvi->eraseFromParent();
NumEliminatedInsts += 2;
return true;
}
}

// Then try to perform the guaranteed copy value optimization.
if (performGuaranteedCopyValueOptimization(cvi))
return true;

return false;
}

//===----------------------------------------------------------------------===//
Expand All @@ -156,31 +179,56 @@ namespace {
// configuration.
struct SemanticARCOpts : SILFunctionTransform {
void run() override {
bool MadeChange = false;
SILFunction *F = getFunction();
if (!F->getModule().isStdlibModule()) {
SILFunction &f = *getFunction();
// Do not run the semantic arc opts unless we are running on the
// standard library. This is because this is the only module that
// passes the ownership verifier.
if (!f.getModule().isStdlibModule())
return;
}

DeadEndBlocks DEBlocks(F);
OwnershipChecker Checker{{}, {}, {}, {}, F->getModule(), DEBlocks};

// First as a special case, handle guaranteed SIL function arguments.
// Iterate over all of the arguments, performing small peephole
// ARC optimizations.
//
// The reason that this is special is that we do not need to consider the
// end of the borrow scope since the end of the function is the end of the
// borrow scope.
for (auto *Arg : F->getArguments()) {
if (Arg->getOwnershipKind() != ValueOwnershipKind::Guaranteed)
// FIXME: Should we iterate or use a RPOT order here?
bool madeChange = false;
for (auto &bb : f) {
auto ii = bb.rend();
auto start = bb.rbegin();

// If the bb is empty, continue.
if (start == ii)
continue;
MadeChange |= optimizeGuaranteedArgument(Arg, Checker);

// Go to the first instruction to process.
--ii;

// Then until we process the first instruction of the block...
while (ii != start) {
// Move the iterator before ii.
auto tmp = std::next(ii);

// Then try to optimize. If we succeeded, then we deleted
// ii. Move ii from the next value back onto the instruction
// after ii's old value in the block instruction list and then
// process that.
if (SemanticARCOptVisitor().visit(&*ii)) {
madeChange = true;
ii = std::prev(tmp);
continue;
}

// Otherwise, we didn't delete ii. Just visit the next instruction.
--ii;
}

// Finally visit the first instruction of the block.
madeChange |= SemanticARCOptVisitor().visit(&*ii);
}

if (MadeChange) {
if (madeChange) {
invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
}
}

};

} // end anonymous namespace
Expand Down
Loading