Skip to content

Fix BorrowingOperand more. #38183

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 1 commit into from
Jul 1, 2021
Merged

Fix BorrowingOperand more. #38183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 19 additions & 27 deletions include/swift/SIL/OwnershipUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@ struct BorrowedValue;
/// borrowed and thus the incoming value must implicitly be borrowed until the
/// user's corresponding end scope instruction.
///
/// Invariant: For a given operand, BorrowingOperand is valid iff
/// it has OperandOwnership::Borrow or OperandOwnership::Reborrow.
///
/// NOTE: We do not require that the guaranteed scope be represented by a
/// guaranteed value in the same function: see begin_apply. In such cases, we
/// require instead an end_* instruction to mark the end of the scope's region.
Expand All @@ -264,10 +267,14 @@ struct BorrowingOperand {

BorrowingOperand(Operand *op)
: op(op), kind(BorrowingOperandKind::get(op->getUser()->getKind())) {
if (kind == BorrowingOperandKind::Branch
&& op->get().getOwnershipKind() != OwnershipKind::Guaranteed) {
auto ownership = op->getOperandOwnership();
if (ownership != OperandOwnership::Borrow
&& ownership != OperandOwnership::Reborrow) {
// consuming applies and branch arguments are not borrowing operands.
kind = BorrowingOperandKind::Invalid;
return;
}
assert(kind != BorrowingOperandKind::Invalid && "missing case");
Copy link
Contributor

Choose a reason for hiding this comment

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

With this can we now eliminate other kinds in BorrowingOperandKind: BeginApply, TryApply etc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@meg-gupta A guaranteed apply argument is still considered a "Borrow" here. I think BorrowingOperand is useful for BeginApply because that's a scoped instruction, so it's important to call visitScopeEndingUses in that case.

I think it is a problem that BorrowingOperand is valid for Apply/TryApply/Yield, but visitScopeEndingUses does nothing there. That will lead to bugs where we miss the use completely. I'll see if it's easy to fix that in this PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it looks like we're always careful before calling visitScopeEndUses to also visit the instruction that begins the scope too. So the behavior doesn't need to be changed, but I can add a comment in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

}
BorrowingOperand(const BorrowingOperand &other)
: op(other.op), kind(other.kind) {}
Expand All @@ -284,28 +291,7 @@ struct BorrowingOperand {
const Operand *operator->() const { return op; }
Operand *operator->() { return op; }

operator bool() const { return kind != BorrowingOperandKind::Invalid && op; }

/// If \p op is a borrow introducing operand return it after doing some
/// checks.
static BorrowingOperand get(Operand *op) {
auto *user = op->getUser();
auto kind = BorrowingOperandKind::get(user->getKind());
if (!kind)
return {nullptr, kind};

if (kind == BorrowingOperandKind::Branch
&& op->get().getOwnershipKind() != OwnershipKind::Guaranteed) {
return {nullptr, BorrowingOperandKind::Invalid};
}
return {op, kind};
}

/// If \p op is a borrow introducing operand return it after doing some
/// checks.
static BorrowingOperand get(const Operand *op) {
return get(const_cast<Operand *>(op));
}
operator bool() const { return kind != BorrowingOperandKind::Invalid; }

/// If this borrowing operand results in the underlying value being borrowed
/// over a region of code instead of just for a single instruction, visit
Expand Down Expand Up @@ -347,8 +333,14 @@ struct BorrowingOperand {
llvm_unreachable("Covered switch isn't covered?!");
}

/// Return true if the user instruction introduces a borrow scope? This is
/// true for both reborrows and nested borrows.
/// Return true if the user instruction defines a borrowed value that
/// introduces a borrow scope and therefore may be reborrowed. This is true
/// for both reborrows and nested borrows.
///
/// Note that begin_apply does create a borrow scope, and may define
/// guaranteed value within that scope. The difference is that those yielded
/// values do not themselves introduce a borrow scope. In other words, they
/// cannot be reborrowed.
///
/// If true, the visitBorrowIntroducingUserResults() can be called to acquire
/// each BorrowedValue that introduces a new borrow scopes.
Expand Down Expand Up @@ -599,7 +591,7 @@ struct BorrowedValue {
&foundReborrows) const {
bool foundAnyReborrows = false;
for (auto *op : value->getUses()) {
if (auto borrowingOperand = BorrowingOperand::get(op)) {
if (auto borrowingOperand = BorrowingOperand(op)) {
if (borrowingOperand.isReborrow()) {
foundReborrows.push_back(
{value->getParentBlock(), op->getOperandNumber()});
Expand Down
2 changes: 1 addition & 1 deletion lib/SIL/IR/OperandOwnership.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ bool swift::checkOperandOwnershipInvariants(const Operand *operand) {
OperandOwnership opOwnership = operand->getOperandOwnership();
if (opOwnership == OperandOwnership::Borrow) {
// Must be a valid BorrowingOperand.
return bool(BorrowingOperand::get(operand));
return bool(BorrowingOperand(const_cast<Operand *>(operand)));
}
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/SIL/Verifier/LinearLifetimeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ void State::checkForSameBlockUseAfterFree(Operand *consumingUse,
}) == userBlock->end()) {
continue;
}
} else if (auto borrowingOperand = BorrowingOperand::get(nonConsumingUse)) {
} else if (auto borrowingOperand = BorrowingOperand(nonConsumingUse)) {
assert(borrowingOperand.isReborrow());
// a reborrow is expected to be consumed by the same phi.
continue;
Expand Down
4 changes: 2 additions & 2 deletions lib/SIL/Verifier/SILOwnershipVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ bool SILValueOwnershipChecker::gatherNonGuaranteedUsers(
// regular users so we can ensure that the borrow scope operand's scope is
// completely within the owned value's scope. If we do not have a borrow
// scope operand, just continue, we are done.
auto initialScopedOperand = BorrowingOperand::get(op);
auto initialScopedOperand = BorrowingOperand(op);
if (!initialScopedOperand) {
continue;
}
Expand Down Expand Up @@ -348,7 +348,7 @@ bool SILValueOwnershipChecker::gatherUsers(
// Ok, our operand does not consume guaranteed values. Check if it is a
// BorrowScopeOperand and if so, add its end scope instructions as
// implicit regular users of our value.
if (auto scopedOperand = BorrowingOperand::get(op)) {
if (auto scopedOperand = BorrowingOperand(op)) {
assert(!scopedOperand.isReborrow());

std::function<void(Operand *)> onError = [&](Operand *op) {
Expand Down
2 changes: 1 addition & 1 deletion lib/SILOptimizer/SemanticARC/CopyValueOpts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ static bool tryJoinIfDestroyConsumingUseInSameBlock(
// we need to only find scopes that end within the region in between the
// singleConsumingUse (the original forwarded use) and the destroy_value. In
// such a case, we must bail!
if (auto operand = BorrowingOperand::get(use))
if (auto operand = BorrowingOperand(use))
if (!operand.visitScopeEndingUses([&](Operand *endScopeUse) {
// Return false if we did see the relevant end scope instruction
// in the block. That means that we are going to exit early and
Expand Down
4 changes: 2 additions & 2 deletions lib/SILOptimizer/Utils/OwnershipOptUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ static void eliminateReborrowsOfRecursiveBorrows(
// Otherwise, we have a reborrow. For now our reborrows must be
// phis. Add our owned value as a new argument of that phi along our
// edge and undef along all other edges.
auto borrowingOp = BorrowingOperand::get(use);
auto borrowingOp = BorrowingOperand(use);
auto *brInst = cast<BranchInst>(borrowingOp.op->getUser());
auto *newBorrowedPhi = brInst->getArgForOperand(*borrowingOp);
auto *newBasePhi =
Expand Down Expand Up @@ -572,7 +572,7 @@ rewriteReborrows(SILValue newBorrowedValue,
// Otherwise, we have a reborrow. For now our reborrows must be
// phis. Add our owned value as a new argument of that phi along our
// edge and undef along all other edges.
auto borrowingOp = BorrowingOperand::get(use);
auto borrowingOp = BorrowingOperand(use);
auto *brInst = cast<BranchInst>(borrowingOp.op->getUser());
auto *newBorrowedPhi = brInst->getArgForOperand(*borrowingOp);
auto *newBasePhi =
Expand Down