Skip to content

[WIP] SemanticARCOpts: Don't copy let properties of classes. #26810

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.

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 1 commit into from
Aug 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions include/swift/SIL/MemAccessUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ class AccessedStorage {
llvm_unreachable("unhandled kind");
}

bool isLetAccess(SILFunction *F) const;

bool isUniquelyIdentified() const {
switch (getKind()) {
case Box:
Expand Down
10 changes: 5 additions & 5 deletions lib/SIL/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -462,15 +462,15 @@ AccessedStorage swift::findAccessedStorageNonNested(SILValue sourceAddr) {
}

// Return true if the given access is on a 'let' lvalue.
static bool isLetAccess(const AccessedStorage &storage, SILFunction *F) {
if (auto *decl = dyn_cast_or_null<VarDecl>(storage.getDecl()))
bool AccessedStorage::isLetAccess(SILFunction *F) const {
if (auto *decl = dyn_cast_or_null<VarDecl>(getDecl()))
return decl->isLet();

// It's unclear whether a global will ever be missing it's varDecl, but
// technically we only preserve it for debug info. So if we don't have a decl,
// check the flag on SILGlobalVariable, which is guaranteed valid,
if (storage.getKind() == AccessedStorage::Global)
return storage.getGlobal()->isLet();
if (getKind() == AccessedStorage::Global)
return getGlobal()->isLet();

return false;
}
Expand Down Expand Up @@ -560,7 +560,7 @@ bool swift::isPossibleFormalAccessBase(const AccessedStorage &storage,
// Additional checks that apply to anything that may fall through.

// Immutable values are only accessed for initialization.
if (isLetAccess(storage, F))
if (storage.isLetAccess(F))
return false;

return true;
Expand Down
106 changes: 97 additions & 9 deletions lib/SILOptimizer/Mandatory/SemanticARCOpts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#define DEBUG_TYPE "sil-semantic-arc-opts"
#include "swift/Basic/STLExtras.h"
#include "swift/SIL/BasicBlockUtils.h"
#include "swift/SIL/BranchPropagatedUser.h"
#include "swift/SIL/MemAccessUtils.h"
#include "swift/SIL/OwnershipUtils.h"
#include "swift/SIL/SILArgument.h"
Expand Down Expand Up @@ -122,10 +123,23 @@ namespace {

struct SemanticARCOptVisitor
: SILInstructionVisitor<SemanticARCOptVisitor, bool> {
SILFunction &F;
Optional<DeadEndBlocks> TheDeadEndBlocks;

explicit SemanticARCOptVisitor(SILFunction &F) : F(F) {}

DeadEndBlocks &getDeadEndBlocks() {
if (!TheDeadEndBlocks)
TheDeadEndBlocks.emplace(&F);
return *TheDeadEndBlocks;
}

bool visitSILInstruction(SILInstruction *i) { return false; }
bool visitCopyValueInst(CopyValueInst *cvi);
bool visitBeginBorrowInst(BeginBorrowInst *bbi);
bool visitLoadInst(LoadInst *li);

bool isWrittenTo(LoadInst *li);
};

} // end anonymous namespace
Expand Down Expand Up @@ -367,10 +381,12 @@ bool mayFunctionMutateArgument(const AccessedStorage &storage, SILFunction &f) {
return true;
}

static bool isWrittenTo(SILFunction &f, SILValue value) {
bool SemanticARCOptVisitor::isWrittenTo(LoadInst *load) {
auto addr = load->getOperand();

// Then find our accessed storage. If we can not find anything, be
// conservative and assume that the value is written to.
const auto &storage = findAccessedStorageNonNested(value);
const auto &storage = findAccessedStorageNonNested(addr);
if (!storage)
return true;

Expand All @@ -379,17 +395,86 @@ static bool isWrittenTo(SILFunction &f, SILValue value) {
// memory). We have to do this since load_borrow assumes that the
// underlying memory is never written to.
switch (storage.getKind()) {
case AccessedStorage::Class: {
// We know a let property won't be written to if the base object is
// guaranteed for the duration of the access.
if (!storage.isLetAccess(&F))
return true;

auto baseObject = stripCasts(storage.getObject());
// A guaranteed argument trivially keeps the base alive for the duration of
// the projection.
if (auto *arg = dyn_cast<SILFunctionArgument>(baseObject)) {
if (arg->getArgumentConvention().isGuaranteedConvention()) {
return false;
}
}

// See if there's a borrow of the base object our load is based on.
SILValue borrowInst;
if (isa<BeginBorrowInst>(baseObject)
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 be extracted out into a helper function or should be constructed in a closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not until it's used somewhere else, IMO. Inline code can't have multiple dependencies.

|| isa<LoadBorrowInst>(baseObject)) {
borrowInst = baseObject;
} else {
// TODO: We should walk the projection path again to get to the
// originating borrow, if any

BeginBorrowInst *singleBorrow = nullptr;
for (auto *use : baseObject->getUses()) {
if (auto *borrow = dyn_cast<BeginBorrowInst>(use->getUser())) {
if (!singleBorrow) {
singleBorrow = borrow;
} else {
singleBorrow = nullptr;
break;
}
}
}

borrowInst = singleBorrow;
}

// Use the linear lifetime checker to check whether the copied
// value is dominated by the lifetime of the borrow it's based on.
if (!borrowInst)
return true;

SmallVector<BranchPropagatedUser, 4> baseEndBorrows;
for (auto *use : borrowInst->getUses()) {
if (isa<EndBorrowInst>(use->getUser())) {
baseEndBorrows.push_back(BranchPropagatedUser(use->getUser()));
}
}

SmallVector<BranchPropagatedUser, 4> valueDestroys;
for (auto *use : load->getUses()) {
if (isa<DestroyValueInst>(use->getUser())) {
valueDestroys.push_back(BranchPropagatedUser(use->getUser()));
}
}

SmallPtrSet<SILBasicBlock *, 4> visitedBlocks;

return valueHasLinearLifetime(baseObject, baseEndBorrows, valueDestroys,
visitedBlocks, getDeadEndBlocks(),
ownership::ErrorBehaviorKind::ReturnFalse)
.getFoundError();
}
case AccessedStorage::Global:
// Any legal load of a global let should have happened after its
// initialization, at which point it can't be written to again for the
// lifetime of the program.
return !storage.isLetAccess(&F);

case AccessedStorage::Box:
case AccessedStorage::Stack:
case AccessedStorage::Global:
case AccessedStorage::Class:
case AccessedStorage::Yield:
case AccessedStorage::Nested:
case AccessedStorage::Unidentified:
return true;

case AccessedStorage::Argument:
return mayFunctionMutateArgument(storage, f);
return mayFunctionMutateArgument(storage, F);
}
llvm_unreachable("covered switch");
}
Expand All @@ -413,7 +498,7 @@ bool SemanticARCOptVisitor::visitLoadInst(LoadInst *li) {

// Then check if our address is ever written to. If it is, then we
// can not use the load_borrow.
if (isWrittenTo(*li->getFunction(), li->getOperand()))
if (isWrittenTo(li))
return false;

// Ok, we can perform our optimization. Convert the load [copy] into a
Expand Down Expand Up @@ -462,6 +547,9 @@ struct SemanticARCOpts : SILFunctionTransform {
//
// FIXME: Should we iterate or use a RPOT order here?
bool madeChange = false;

SemanticARCOptVisitor visitor(f);

for (auto &bb : f) {
auto ii = bb.rend();
auto start = bb.rbegin();
Expand All @@ -482,7 +570,7 @@ struct SemanticARCOpts : SILFunctionTransform {
// 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)) {
if (visitor.visit(&*ii)) {
madeChange = true;
ii = std::prev(tmp);
continue;
Expand All @@ -493,7 +581,7 @@ struct SemanticARCOpts : SILFunctionTransform {
}

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

if (madeChange) {
Expand Down
Loading