Skip to content

[NFC] Move doesDestructorHaveSideEffects to InstOptUtils. #31681

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

Closed
wants to merge 1 commit into from
Closed

[NFC] Move doesDestructorHaveSideEffects to InstOptUtils. #31681

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
10 changes: 10 additions & 0 deletions include/swift/SILOptimizer/Utils/InstOptUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,16 @@ FullApplySite cloneFullApplySiteReplacingCallee(FullApplySite applySite,
SILValue newCallee,
SILBuilderContext &builderCtx);

/// Analyze the destructor for the class of \p value to see if any instructions in it could have side effects on
/// the program outside the destructor. If the destructor is not a class or cannot be analyzed returns true.
bool doesDestructorHaveSideEffects(SILValue value);

/// Returns the destructor of the class \p value if it is visiable and can be analyzed. If the \p value is not
/// a class, is not visiable, or cannot be analyzed, returns nullptr.
///
/// Note: analyzing the body of this class destructor is valid because the object is dead. This means that the
/// object is never passed to objc_setAssociatedObject, so its destructor cannot be extended at runtime.
SILFunction *getDestructor(SILValue value);
Copy link
Contributor

@eeckstein eeckstein May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to pass a SILValue instead of an AllocRefInst.
In case of anything else than an alloc_ref it's not guaranteed that the returned destructor is really the destructor which is called. It could also be the destructor of a derived class.
So it's easy to misuse this API function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. The use case I have in mind is getting the destructor of the operand of a release and passing it to doesDestructorHaveSideEffects. Maybe we could inline it into doesDestructorHaveSideEffects or add a comment that users should be careful? What do you think a good solution would be?

Copy link
Contributor

Choose a reason for hiding this comment

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

What you can do is: check if it's an alloc_ref or a final class and otherwise return null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I'm wrong. Unfortunately there is this thing with associated objects. A class instance can have an associated object, which can have any kind of side effects on destruction.
So even a final class or if value is an alloc_ref, you cannot be sure about the side effects of the destructor.
It only works in dead object elimination, because if the object is dead, it's guaranteed that it's never passed to a call to setAssociatedObject.

Sorry, I didn't think of that. The conclusion is that generalizing this API is problematic because the user of doesDestructorHaveSideEffects must make sure that the value does not have an associated object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. That makes sense. I'll close it.

} // end namespace swift

#endif
121 changes: 0 additions & 121 deletions lib/SILOptimizer/Transforms/DeadObjectElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,127 +61,6 @@ STATISTIC(DeadAllocApplyEliminated,

using UserList = llvm::SmallSetVector<SILInstruction *, 16>;

// Analyzing the body of this class destructor is valid because the object is
// dead. This means that the object is never passed to objc_setAssociatedObject,
// so its destructor cannot be extended at runtime.
static SILFunction *getDestructor(AllocRefInst *ARI) {
// We only support classes.
ClassDecl *ClsDecl = ARI->getType().getClassOrBoundGenericClass();
if (!ClsDecl)
return nullptr;

// Look up the destructor of ClsDecl.
DestructorDecl *Destructor = ClsDecl->getDestructor();
assert(Destructor && "getDestructor() should never return a nullptr.");

// Find the destructor name via SILDeclRef.
// FIXME: When destructors get moved into vtables, update this to use the
// vtable for the class.
SILDeclRef Ref(Destructor);
SILFunction *Fn = ARI->getModule().lookUpFunction(Ref);
if (!Fn || Fn->empty()) {
LLVM_DEBUG(llvm::dbgs() << " Could not find destructor.\n");
return nullptr;
}

LLVM_DEBUG(llvm::dbgs() << " Found destructor!\n");

// If the destructor has an objc_method calling convention, we cannot
// analyze it since it could be swapped out from under us at runtime.
if (Fn->getRepresentation() == SILFunctionTypeRepresentation::ObjCMethod) {
LLVM_DEBUG(llvm::dbgs() << " Found Objective-C destructor. Can't "
"analyze!\n");
return nullptr;
}

return Fn;
}

/// Analyze the destructor for the class of ARI to see if any instructions in it
/// could have side effects on the program outside the destructor. If it does
/// not, then we can eliminate the destructor.
static bool doesDestructorHaveSideEffects(AllocRefInst *ARI) {
SILFunction *Fn = getDestructor(ARI);
// If we can't find a constructor then assume it has side effects.
if (!Fn)
return true;

// A destructor only has one argument, self.
assert(Fn->begin()->getNumArguments() == 1 &&
"Destructor should have only one argument, self.");
SILArgument *Self = Fn->begin()->getArgument(0);

LLVM_DEBUG(llvm::dbgs() << " Analyzing destructor.\n");

// For each BB in the destructor...
for (auto &BB : *Fn)
// For each instruction I in BB...
for (auto &I : BB) {
LLVM_DEBUG(llvm::dbgs() << " Visiting: " << I);

// If I has no side effects, we can ignore it.
if (!I.mayHaveSideEffects()) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! Instruction has no side "
"effects.\n");
continue;
}

// RefCounting operations on Self are ok since we are already in the
// destructor. RefCountingOperations on other instructions could have side
// effects though.
if (auto *RefInst = dyn_cast<RefCountingInst>(&I)) {
if (stripCasts(RefInst->getOperand(0)) == Self) {
// For now all ref counting insts have 1 operand. Put in an assert
// just in case.
assert(RefInst->getNumOperands() == 1 &&
"Make sure RefInst only has one argument.");
LLVM_DEBUG(llvm::dbgs() << " SAFE! Ref count operation on "
"Self.\n");
continue;
} else {
LLVM_DEBUG(llvm::dbgs() << " UNSAFE! Ref count operation "
"not on self.\n");
return true;
}
}

// dealloc_stack can be ignored.
if (isa<DeallocStackInst>(I)) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! dealloc_stack can be "
"ignored.\n");
continue;
}

// dealloc_ref on self can be ignored, but dealloc_ref on anything else
// cannot be eliminated.
if (auto *DeallocRef = dyn_cast<DeallocRefInst>(&I)) {
if (stripCasts(DeallocRef->getOperand()) == Self) {
LLVM_DEBUG(llvm::dbgs() <<" SAFE! dealloc_ref on self.\n");
continue;
} else {
LLVM_DEBUG(llvm::dbgs() << " UNSAFE! dealloc_ref on value "
"besides self.\n");
return true;
}
}

// Storing into the object can be ignored.
if (auto *SI = dyn_cast<StoreInst>(&I))
if (stripAddressProjections(SI->getDest()) == Self) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! Instruction is a store "
"into self.\n");
continue;
}

LLVM_DEBUG(llvm::dbgs() << " UNSAFE! Unknown instruction.\n");
// Otherwise, we can't remove the deallocation completely.
return true;
}

// We didn't find any side effects.
return false;
}

void static
removeInstructions(ArrayRef<SILInstruction*> UsersToRemove) {
for (auto *I : UsersToRemove) {
Expand Down
117 changes: 117 additions & 0 deletions lib/SILOptimizer/Utils/InstOptUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

#define DEBUG_TYPE "inst-opt-utils"
#include "swift/SILOptimizer/Utils/InstOptUtils.h"
#include "swift/AST/GenericSignature.h"
#include "swift/AST/SemanticAttrs.h"
Expand Down Expand Up @@ -2042,3 +2043,119 @@ swift::cloneFullApplySiteReplacingCallee(FullApplySite applySite,
}
llvm_unreachable("Unhandled case?!");
}

SILFunction *swift::getDestructor(SILValue value) {
// We only support classes.
ClassDecl *ClsDecl = value->getType().getClassOrBoundGenericClass();
if (!ClsDecl)
return nullptr;

// Look up the destructor of ClsDecl.
DestructorDecl *Destructor = ClsDecl->getDestructor();
assert(Destructor && "getDestructor() should never return a nullptr.");

// Find the destructor name via SILDeclRef.
// FIXME: When destructors get moved into vtables, update this to use the
// vtable for the class.
SILDeclRef Ref(Destructor);
SILFunction *Fn = value->getModule()->lookUpFunction(Ref);
if (!Fn || Fn->empty()) {
LLVM_DEBUG(llvm::dbgs() << " Could not find destructor.\n");
return nullptr;
}

LLVM_DEBUG(llvm::dbgs() << " Found destructor!\n");

// If the destructor has an objc_method calling convention, we cannot
// analyze it since it could be swapped out from under us at runtime.
if (Fn->getRepresentation() == SILFunctionTypeRepresentation::ObjCMethod) {
LLVM_DEBUG(llvm::dbgs() << " Found Objective-C destructor. Can't "
"analyze!\n");
return nullptr;
}

return Fn;
}

bool swift::doesDestructorHaveSideEffects(SILValue value) {
SILFunction *Fn = getDestructor(value);
// If we can't find a constructor then assume it has side effects.
if (!Fn)
return true;

// A destructor only has one argument, self.
assert(Fn->begin()->getNumArguments() == 1 &&
"Destructor should have only one argument, self.");
SILArgument *Self = Fn->begin()->getArgument(0);

LLVM_DEBUG(llvm::dbgs() << " Analyzing destructor.\n");

// For each BB in the destructor...
for (auto &BB : *Fn)
// For each instruction I in BB...
for (auto &I : BB) {
LLVM_DEBUG(llvm::dbgs() << " Visiting: " << I);

// If I has no side effects, we can ignore it.
if (!I.mayHaveSideEffects()) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! Instruction has no side "
"effects.\n");
continue;
}

// RefCounting operations on Self are ok since we are already in the
// destructor. RefCountingOperations on other instructions could have side
// effects though.
if (auto *RefInst = dyn_cast<RefCountingInst>(&I)) {
if (stripCasts(RefInst->getOperand(0)) == Self) {
// For now all ref counting insts have 1 operand. Put in an assert
// just in case.
assert(RefInst->getNumOperands() == 1 &&
"Make sure RefInst only has one argument.");
LLVM_DEBUG(llvm::dbgs() << " SAFE! Ref count operation on "
"Self.\n");
continue;
} else {
LLVM_DEBUG(llvm::dbgs() << " UNSAFE! Ref count operation "
"not on self.\n");
return true;
}
}

// dealloc_stack can be ignored.
if (isa<DeallocStackInst>(I)) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! dealloc_stack can be "
"ignored.\n");
continue;
}

// dealloc_ref on self can be ignored, but dealloc_ref on anything else
// cannot be eliminated.
if (auto *DeallocRef = dyn_cast<DeallocRefInst>(&I)) {
if (stripCasts(DeallocRef->getOperand()) == Self) {
LLVM_DEBUG(llvm::dbgs()
<< " SAFE! dealloc_ref on self.\n");
continue;
} else {
LLVM_DEBUG(llvm::dbgs() << " UNSAFE! dealloc_ref on value "
"besides self.\n");
return true;
}
}

// Storing into the object can be ignored.
if (auto *SI = dyn_cast<StoreInst>(&I))
if (stripAddressProjections(SI->getDest()) == Self) {
LLVM_DEBUG(llvm::dbgs() << " SAFE! Instruction is a store "
"into self.\n");
continue;
}

LLVM_DEBUG(llvm::dbgs() << " UNSAFE! Unknown instruction.\n");
// Otherwise, we can't remove the deallocation completely.
return true;
}

// We didn't find any side effects.
return false;
}