Skip to content

[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

Closed
wants to merge 5 commits into from

Conversation

eeckstein
Copy link
Contributor

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

@eeckstein eeckstein requested a review from a team as a code owner April 12, 2023 06:21
@eeckstein eeckstein requested a review from gottesmm April 12, 2023 06:21
@eeckstein
Copy link
Contributor Author

@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.
@eeckstein
Copy link
Contributor Author

@swift-ci test

@tbkka tbkka requested review from jckarter and kavon April 13, 2023 18:43
Copy link
Member

@kavon kavon left a 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

Comment on lines +1225 to +1228
void visitDropDeinitInst(DropDeinitInst *i) {
auto e = getLoweredExplosion(i->getOperand());
setLoweredExplosion(i, e);
}
Copy link
Member

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?

Copy link
Contributor Author

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

@AnthonyLatsis AnthonyLatsis added the 🍒 release cherry pick Flag: Release branch cherry picks label May 3, 2023
@eeckstein eeckstein closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants