-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[5.9] Add the drop_deinit instruction #65094
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
Conversation
@swift-ci test |
his instruction is a marker for a following destroy instruction to suppress the call of the move-only type's deinitializer.
…gument This happens for address-only move-only types.
…eeded by a drop_deinit rdar://105798769
33ed3d9
to
dbc9f74
Compare
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I see no risks in merging this into 5.9 and it looks good. Had a few comments that are mostly clarifications since i missed reviewing this when it landed into main
void visitDropDeinitInst(DropDeinitInst *i) { | ||
auto e = getLoweredExplosion(i->getOperand()); | ||
setLoweredExplosion(i, e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the instruction is only valid in OSSA, isn't this not necessary because reaching IRGen is unexpected / an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. This could be an unreachable.
I just copied from visitMoveValueInst
.
But you are right, we should replace both visit functions with unreachable
@@ -2385,7 +2385,7 @@ struct DeallocatorConventions : Conventions { | |||
|
|||
ParameterConvention | |||
getIndirectSelfParameter(const AbstractionPattern &type) const override { | |||
llvm_unreachable("Deallocators do not have indirect self parameters"); | |||
return ParameterConvention::Indirect_In; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about why this change was needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for supporting deinits which get the self argument as indirect argument, which is the case for move-only types.
The
drop_deinit
instruction is a marker for a following destroy instruction to suppress the call of the move-only type's deinitializer.SILGen needs to insert
drop_deinit
in move-only deinitializers to avoid that - due to other optimizations, like inlining - the deinitializer call is inserted multiple times.rdar://105798769
This is a cherry-pick of #65060