[NFC] Move doesDestructorHaveSideEffects to InstOptUtils. #31681
Conversation
|
As @eeckstein suggested in #31134 I'd like to use this in MemoryBehaviorVisitor / EscapeAnalysis. |
|
Friendly ping. This PR just moves |
| @@ -587,6 +587,14 @@ 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. | |||
There was a problem hiding this comment.
Can you add something like "Conservatively returns true if the destructor cannot be analyzed or if value is not a class"
|
|
||
| /// 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. |
There was a problem hiding this comment.
This comment is not very good (It was wrong in the original code, too).
It should be something like, "Returns the destructor of \p value ... Returns null if value is not a class or the destructor is not visible or cannot be analyzed"
| /// 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What you can do is: check if it's an alloc_ref or a final class and otherwise return null.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No worries. That makes sense. I'll close it.
|
@eeckstein Thanks for the review! Comments have been improved :) |
Moves doesDestructorHaveSideEffects and getDestructor to InstOptUtils from DeadObjectElimination. The argument types were changed from AllocRefInst* to SILValue.
zoecarver
force-pushed
the
lva/general-destructor-analysis
branch
from
aca580e to
da42a9c
Compare
|
I just noticed that we also have the DestructorAnalysis, which does the same thing for structs and tuples, but not for classes. So it's probably a good idea to include doesDestructorHaveSideEffects into DestructorAnalysis and use DestructorAnalysis is DeadObjectElimination. Update: This does not work because of associated objects (see my other comment) |
Moves
doesDestructorHaveSideEffectsandgetDestructortoInstOptUtilsfromDeadObjectElimination.The argument types were changed from
AllocRefInst*toSILValue.I also ran clang-format over the changes.