Skip to content

[CopyForwarding] Removed destroy hoisting. #41377

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
Feb 19, 2022
Merged

[CopyForwarding] Removed destroy hoisting. #41377

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
a091814
[Gardening] Removed extraneous import.
nate-chandler Feb 11, 2022
4e2667b
[CopyForwarding] Removed destroy hoisting.
nate-chandler Feb 11, 2022
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
227 changes: 12 additions & 215 deletions lib/SILOptimizer/Transforms/CopyForwarding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ using namespace swift;
// Temporary debugging flag until this pass is better tested.
static llvm::cl::opt<bool> EnableCopyForwarding("enable-copyforwarding",
llvm::cl::init(true));
static llvm::cl::opt<bool> EnableDestroyHoisting("enable-destroyhoisting",
llvm::cl::init(true));

/// \return true if the given copy source value can only be accessed via the
/// given def (this def uniquely identifies the object).
Expand Down Expand Up @@ -513,7 +511,6 @@ class CopyForwarding {
PostOrderAnalysis *PostOrder;
DominanceAnalysis *DomAnalysis;
RCIdentityAnalysis *RCIAnalysis;
bool DoGlobalHoisting;
bool HasChanged;
bool HasChangedCFG;

Expand All @@ -535,7 +532,6 @@ class CopyForwarding {
SmallPtrSet<SILInstruction*, 16> SrcUserInsts;
SmallPtrSet<SILInstruction*, 4> SrcDebugValueInsts;
SmallVector<CopyAddrInst*, 4> TakePoints;
SmallPtrSet<SILInstruction *, 16> StoredValueUserInsts;
SmallVector<DestroyAddrInst*, 4> DestroyPoints;
SmallPtrSet<SILBasicBlock*, 32> DeadInBlocks;

Expand Down Expand Up @@ -579,7 +575,7 @@ class CopyForwarding {
CopyForwarding(PostOrderAnalysis *PO, DominanceAnalysis *DA,
RCIdentityAnalysis *RCIAnalysis)
: PostOrder(PO), DomAnalysis(DA), RCIAnalysis(RCIAnalysis),
DoGlobalHoisting(false), HasChanged(false), HasChangedCFG(false),
HasChanged(false), HasChangedCFG(false),
IsSrcLoadedFrom(false), HasUnknownStoredValue(false),
HasForwardedToCopy(false), CurrentCopy(nullptr) {}

Expand All @@ -590,7 +586,6 @@ class CopyForwarding {
// some alloc_stack cases after global destroy hoisting. CopyForwarding will
// be reapplied after the transparent function is inlined at which point
// global hoisting will be done.
DoGlobalHoisting = !F->isTransparent();
if (HasChangedCFG) {
// We are only invalidating the analysis that we use internally.
// We'll invalidate the analysis that are used by other passes at the end.
Expand All @@ -605,7 +600,6 @@ class CopyForwarding {
SrcUserInsts.clear();
SrcDebugValueInsts.clear();
TakePoints.clear();
StoredValueUserInsts.clear();
DestroyPoints.clear();
DeadInBlocks.clear();
CurrentCopy = nullptr;
Expand All @@ -621,15 +615,13 @@ class CopyForwarding {
void forwardCopiesOf(SILValue Def, SILFunction *F);

protected:
bool propagateCopy(CopyAddrInst *CopyInst, bool hoistingDestroy);
bool propagateCopy(CopyAddrInst *CopyInst);
CopyAddrInst *findCopyIntoDeadTemp(
CopyAddrInst *destCopy,
SmallVectorImpl<SILInstruction *> &debugInstsToDelete);
bool forwardDeadTempCopy(CopyAddrInst *destCopy);
bool forwardPropagateCopy();
bool backwardPropagateCopy();
bool hoistDestroy(SILInstruction *DestroyPoint, SILLocation DestroyLoc,
SmallVectorImpl<SILInstruction *> &debugInstsToDelete);

bool isSourceDeadAtCopy();

Expand Down Expand Up @@ -671,7 +663,7 @@ class CopyDestUserVisitor : public AddressUserVisitor {
/// If the forwarded copy is not an [init], then insert a destroy of the copy's
/// dest.
bool CopyForwarding::
propagateCopy(CopyAddrInst *CopyInst, bool hoistingDestroy) {
propagateCopy(CopyAddrInst *CopyInst) {
if (!EnableCopyForwarding)
return false;

Expand All @@ -693,7 +685,7 @@ propagateCopy(CopyAddrInst *CopyInst, bool hoistingDestroy) {

// Handle copy-of-copy without analyzing uses.
// Assumes that CurrentCopy->getSrc() is dead after CurrentCopy.
assert(CurrentCopy->isTakeOfSrc() || hoistingDestroy);
assert(CurrentCopy->isTakeOfSrc());
if (forwardDeadTempCopy(CurrentCopy)) {
HasChanged = true;
++NumDeadTemp;
Expand All @@ -703,8 +695,7 @@ propagateCopy(CopyAddrInst *CopyInst, bool hoistingDestroy) {
if (forwardPropagateCopy()) {
LLVM_DEBUG(llvm::dbgs() << " Forwarding Copy:" << *CurrentCopy);
if (!CurrentCopy->isInitializationOfDest()) {
// Replace the original copy with a destroy. We may be able to hoist it
// more in another pass but don't currently iterate.
// Replace the original copy with a destroy.
SILBuilderWithScope(CurrentCopy)
.createDestroyAddr(CurrentCopy->getLoc(), CurrentCopy->getDest());
}
Expand Down Expand Up @@ -832,16 +823,7 @@ forwardDeadTempCopy(CopyAddrInst *destCopy) {
deadDebugUser->eraseFromParent();
}

// Either `destCopy` is a take, or the caller is hoisting a destroy:
// copy_addr %temp, %dest
// ...
// destroy %temp
//
// If the caller is hoisting a destroy, and we return `true` then it will
// erase the destroy for us. Either way, it's safe to simply rewrite destCopy.
// For now, don't bother finding the subsequent destroy, because this isn't
// the common case.

// `destCopy` is a take. It's safe to simply rewrite destCopy.
destCopy->setSrc(srcCopy->getSrc());
destCopy->setIsTakeOfSrc(srcCopy->isTakeOfSrc());
srcCopy->eraseFromParent();
Expand Down Expand Up @@ -878,8 +860,7 @@ bool CopyForwarding::doesCopyDominateDestUsers(
return true;
}

// Add all recognized users of storedValue to StoredValueUserInsts. Return true
// if all users were recgonized.
// Return true if all users were recgonized.
//
// To find all SSA users of storedValue, we first find the RC root, then search
// past any instructions that may propagate the reference.
Expand All @@ -900,19 +881,16 @@ bool CopyForwarding::markStoredValueUsers(SILValue storedValue) {
// Recognize any uses that have no results as normal uses. They cannot
// transitively propagate a reference.
if (user->getResults().empty()) {
StoredValueUserInsts.insert(user);
continue;
}
// Recognize full applies as normal uses. They may transitively retain, but
// the caller cannot rely on that.
if (FullApplySite::isa(user)) {
StoredValueUserInsts.insert(user);
continue;
}
// A single-valued use is nontransitive if its result is trivial.
if (auto *SVI = dyn_cast<SingleValueInstruction>(user)) {
if (SVI->getType().isTrivial(*F)) {
StoredValueUserInsts.insert(user);
continue;
}
}
Expand Down Expand Up @@ -957,8 +935,7 @@ static DeallocStackInst *getSingleDealloc(AllocStackInst *ASI) {
/// If the last use (deinit) is a copy, replace it with a destroy+copy[init].
///
/// The caller has already guaranteed that the lifetime of the copy's source
/// ends at this copy. Either the copy is a [take] or a destroy can be hoisted
/// to the copy.
/// ends at this copy. The copy is a [take].
bool CopyForwarding::forwardPropagateCopy() {

SILValue CopyDest = CurrentCopy->getDest();
Expand Down Expand Up @@ -1221,98 +1198,6 @@ bool CopyForwarding::backwardPropagateCopy() {
return true;
}

/// Attempt to hoist a destroy point up to the last use. If the last use is a
/// copy, eliminate both the copy and the destroy.
///
/// The copy will be eliminated if the original is not accessed between the
/// point of copy and the original's destruction.
///
/// CurrentDef = <uniquely identified> // no aliases
/// ...
/// Copy = copy_addr [init] Def
/// ... // no access to CurrentDef
/// destroy_addr Def
///
/// Return true if a destroy was inserted, forwarded from a copy, or the
/// block was marked dead-in.
/// \p debugInstsToDelete will contain the debug_value users of copy src
/// that need to be deleted if the hoist is successful
bool CopyForwarding::hoistDestroy(
SILInstruction *DestroyPoint, SILLocation DestroyLoc,
SmallVectorImpl<SILInstruction *> &debugInstsToDelete) {
if (!EnableDestroyHoisting)
return false;

assert(!SrcUserInsts.count(DestroyPoint) && "caller should check terminator");
SILBasicBlock *BB = DestroyPoint->getParent();

// If DestroyPoint is a block terminator, we must hoist.
bool MustHoist = (DestroyPoint == BB->getTerminator());
// If we haven't seen anything significant, avoid useless hoisting.
bool ShouldHoist = MustHoist;

auto tryToInsertHoistedDestroyAfter = [&](SILInstruction *afterInst) {
if (!ShouldHoist)
return false;
LLVM_DEBUG(llvm::dbgs() << " Hoisting to Use:" << *afterInst);
SILBuilderWithScope(std::next(afterInst->getIterator()), afterInst)
.createDestroyAddr(DestroyLoc, CurrentDef);
HasChanged = true;
return true;
};

auto SI = DestroyPoint->getIterator(), SE = BB->begin();
while (SI != SE) {
--SI;
SILInstruction *Inst = &*SI;
if (!SrcUserInsts.count(Inst)) {
if (StoredValueUserInsts.count(Inst)) {
// The current definition may take ownership of a value stored into its
// address. Its lifetime cannot end before the last use of that stored
// value.
// CurrentDef = ...
// Copy = copy_addr CurrentDef to ...
// store StoredValue to CurrentDef
// ... // no access to CurrentDef
// retain StoredValue
// destroy_addr CurrentDef
LLVM_DEBUG(llvm::dbgs() << " Cannot hoist above stored value use:"
<< *Inst);
return tryToInsertHoistedDestroyAfter(Inst);
}
if (isa<DebugValueInst>(Inst)) {
// Collect debug_value uses of copy src. If the hoist is
// successful, these instructions will have consumed operand and need to
// be erased.
if (SrcDebugValueInsts.contains(Inst)) {
debugInstsToDelete.push_back(Inst);
}
}
if (!ShouldHoist && isa<ApplyInst>(Inst))
ShouldHoist = true;
continue;
}
if (auto *CopyInst = dyn_cast<CopyAddrInst>(Inst)) {
if (!CopyInst->isTakeOfSrc() && CopyInst->getSrc() == CurrentDef) {
// This use is a copy of CurrentDef. Attempt to forward CurrentDef to
// all uses of the copy's value.
if (propagateCopy(CopyInst, /*hoistingDestroy=*/true))
return true;
}
}
return tryToInsertHoistedDestroyAfter(Inst);
}
if (!DoGlobalHoisting) {
// If DoGlobalHoisting is set, then we should never mark a DeadInBlock, so
// MustHoist should be false.
assert(!MustHoist &&
"Cannot hoist above a terminator with global hoisting disabled.");
return false;
}
DeadInBlocks.insert(BB);
return true;
}

/// Perform CopyForwarding on the current Def.
void CopyForwarding::forwardCopiesOf(SILValue Def, SILFunction *F) {
reset(F);
Expand All @@ -1322,97 +1207,9 @@ void CopyForwarding::forwardCopiesOf(SILValue Def, SILFunction *F) {
if (!visitAddressUsers(Def, nullptr, visitor))
return;

// First forward any copies that implicitly destroy CurrentDef. There is no
// need to hoist Destroy for these.
// Forward any copies that implicitly destroy CurrentDef.
for (auto *CopyInst : TakePoints) {
propagateCopy(CopyInst, /*hoistingDestroy=*/false);
}
// If the copied address is also loaded from, then destroy hoisting is unsafe.
//
// TODO: Record all loads during collectUsers. Implement findRetainPoints to
// peek though projections of the load, like unchecked_enum_data to find the
// true extent of the lifetime including transitively referenced objects.
if (IsSrcLoadedFrom || HasUnknownStoredValue)
return;

bool HoistedDestroyFound = false;
SILLocation HoistedDestroyLoc = F->getLocation();
const SILDebugScope *HoistedDebugScope = nullptr;
SmallVector<SILInstruction *, 2> debugInstsToDelete;

for (auto *Destroy : DestroyPoints) {
// If hoistDestroy returns false, it was not worth hoisting.
if (hoistDestroy(Destroy, Destroy->getLoc(), debugInstsToDelete)) {
// Propagate DestroyLoc for any destroy hoisted above a block.
if (DeadInBlocks.count(Destroy->getParent())) {
HoistedDestroyLoc = Destroy->getLoc();
HoistedDebugScope = Destroy->getDebugScope();
HoistedDestroyFound = true;
}
// We either just created a new destroy, forwarded a copy, or will
// continue propagating from this dead-in block. In any case, erase the
// original Destroy.
Destroy->eraseFromParent();
assert(HasChanged || !DeadInBlocks.empty() && "HasChanged should be set");
// Since the hoist was successful, delete all dead debug_value
for (auto *deadDebugUser : debugInstsToDelete) {
deadDebugUser->eraseFromParent();
}
}
debugInstsToDelete.clear();
}
// Any blocks containing a DestroyPoints where hoistDestroy did not find a use
// are now marked in DeadInBlocks.
if (DeadInBlocks.empty())
return;

assert(HoistedDestroyFound && "Hoisted destroy should have been found");

DestroyPoints.clear();

// Propagate dead-in blocks upward via PostOrder traversal.
// TODO: We could easily handle hoisting above loops if LoopInfo is available.
//
for (auto *BB : PostOrder->get(F)->getPostOrder()) {
SmallVector<unsigned, 4> DeadInSuccs;
ArrayRef<SILSuccessor> Succs = BB->getSuccessors();
if (Succs.empty())
continue;

for (unsigned EdgeIdx = 0, End = Succs.size(); EdgeIdx != End; ++EdgeIdx) {
if (DeadInBlocks.count(Succs[EdgeIdx].getBB()))
DeadInSuccs.push_back(EdgeIdx);
}
if (DeadInSuccs.size() == Succs.size() &&
!SrcUserInsts.count(BB->getTerminator())) {
// All successors are dead, so continue hoisting.
bool WasHoisted = hoistDestroy(BB->getTerminator(), HoistedDestroyLoc,
debugInstsToDelete);
(void)WasHoisted;
assert(WasHoisted && "should always hoist above a terminator");
for (auto *deadDebugUser : debugInstsToDelete) {
deadDebugUser->eraseFromParent();
}
debugInstsToDelete.clear();
continue;
}
// Emit a destroy on each CFG edge leading to a dead-in block. This requires
// splitting critical edges and will naturally handle redundant branch
// targets.
for (unsigned EdgeIdx : DeadInSuccs) {
SILBasicBlock *SuccBB = splitCriticalEdge(BB->getTerminator(), EdgeIdx);
if (SuccBB)
HasChangedCFG = true;
else
SuccBB = BB->getSuccessors()[EdgeIdx];

// We make no attempt to use the best DebugLoc, because in all known
// cases, we only have one.
SILBuilder B(SuccBB->begin());
B.setCurrentDebugScope(HoistedDebugScope);
B.createDestroyAddr(HoistedDestroyLoc, CurrentDef);
HasChanged = true;
}
propagateCopy(CopyInst);
}
}

Expand Down Expand Up @@ -1539,7 +1336,7 @@ static llvm::cl::opt<int> ForwardStop("copy-forward-stop",
class CopyForwardingPass : public SILFunctionTransform
{
void run() override {
if (!EnableCopyForwarding && !EnableDestroyHoisting)
if (!EnableCopyForwarding)
return;

// This pass assumes that the ownership lifetime of a value in a memory
Expand Down Expand Up @@ -1570,7 +1367,7 @@ class CopyForwardingPass : public SILFunctionTransform
for (auto &BB : *getFunction())
for (auto II = BB.begin(), IE = BB.end(); II != IE; ++II) {
if (auto *CopyInst = dyn_cast<CopyAddrInst>(&*II)) {
if (EnableDestroyHoisting && canNRVO(CopyInst)) {
if (canNRVO(CopyInst)) {
NRVOCopies.push_back(CopyInst);
continue;
}
Expand Down
1 change: 0 additions & 1 deletion lib/SILOptimizer/Utils/LexicalDestroyFolding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
/// TODO: Handle partial_apply, try_apply, and begin_apply.
//===----------------------------------------------------------------------===//

#include "swift/Basic/GraphNodeWorklist.h"
#include "swift/SIL/BasicBlockDatastructures.h"
#include "swift/SIL/BasicBlockUtils.h"
#include "swift/SIL/OwnershipUtils.h"
Expand Down
2 changes: 1 addition & 1 deletion test/SILOptimizer/copyforward.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-sil-opt -enforce-exclusivity=none -enable-sil-verify-all %s -copy-forwarding -enable-copyforwarding -enable-destroyhoisting | %FileCheck %s
// RUN: %target-sil-opt -enforce-exclusivity=none -enable-sil-verify-all %s -copy-forwarding -enable-copyforwarding | %FileCheck %s

// CopyForwarding currently only runs on OSSA. This file only contains
// tests that will break is CopyForwarding were to run on non-OSSA SIL.
Expand Down
Loading