Skip to content

CSE/OSSA avoid endless cloning and reoptimization #39736

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 2 commits into from
Oct 14, 2021
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
19 changes: 13 additions & 6 deletions include/swift/SILOptimizer/Utils/OwnershipOptUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,10 @@ struct OwnershipFixupContext {
/// and use this to seed that new lifetime.
SmallVector<Operand *, 8> allAddressUsesFromOldValue;

/// This is the interior pointer operand that the new value we want to RAUW
/// is transitively derived from and enables us to know the underlying
/// borrowed base value that we need to lifetime extend.
/// This is the interior pointer (e.g. ref_element_addr)
/// that the new value we want to RAUW is transitively derived from and
/// enables us to know the underlying borrowed base value that we need to
/// lifetime extend.
///
/// This is only initialized when the interior pointer has uses that must be
/// replaced.
Expand Down Expand Up @@ -149,9 +150,15 @@ class OwnershipRAUWHelper {
operator bool() const { return isValid(); }
bool isValid() const { return bool(ctx) && bool(oldValue) && bool(newValue); }

/// Perform the actual RAUW. We require that \p newValue and \p oldValue have
/// the same type at this point (in contrast to when calling
/// OwnershipRAUWFixupHelper::get()).
/// True if replacement requires copying the original instruction's source
/// operand, creating a new borrow scope for that copy, then cloning the
/// original.
bool requiresCopyBorrowAndClone() const {
return ctx->extraAddressFixupInfo.base;
}

/// Perform OSSA fixup on newValue and return a fixed-up value based that can
/// be used to replace all uses of oldValue.
///
/// This is so that we can avoid creating "forwarding" transformation
/// instructions before we know if we can perform the RAUW. Any such
Expand Down
5 changes: 4 additions & 1 deletion lib/SILOptimizer/Transforms/CSE.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,10 @@ bool CSE::processNode(DominanceInfoNode *Node) {
OwnershipRAUWHelper helper(RAUWFixupContext,
cast<SingleValueInstruction>(Inst),
cast<SingleValueInstruction>(AvailInst));
if (!helper.isValid())
// If RAUW requires cloning the original, then there's no point. If it
// also requires introducing a copy and new borrow scope, then it's a
// very bad idea.
if (!helper.isValid() || helper.requiresCopyBorrowAndClone())
continue;
// Replace SingleValueInstruction using OSSA RAUW here
nextI = helper.perform();
Expand Down
7 changes: 3 additions & 4 deletions lib/SILOptimizer/Utils/OwnershipOptUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1041,10 +1041,9 @@ OwnershipRAUWHelper::replaceAddressUses(SingleValueInstruction *oldValue,
SILValue newValue) {
assert(oldValue->getType().isAddress() && newValue->getType().isAddress());

// If we are replacing addresses, see if we need to handle interior pointer
// fixups. If we don't have any extra info, then we know that we can just RAUW
// without any further work.
if (!ctx->extraAddressFixupInfo.base)
// If newValue was not generated by an interior pointer, then it cannot
// be within a borrow scope, so direct replacement works.
if (!requiresCopyBorrowAndClone())
return replaceAllUsesAndErase(oldValue, newValue, ctx->callbacks);

// We are RAUWing two addresses and we found that:
Expand Down