Skip to content

Fix forwardingOwnershipKind of all OwnershipForwardingMixin's while cloning #36143

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

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

meg-gupta
Copy link
Contributor

forwardingOwnershipKind can differ from the operand's ownershipKind. We
need to copy forwardingOwnershipKind while cloning these instructions.

Also print the forwarding ownership kind when it differs from its
operand's ownershipKind

This is a follow up of #36063

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta meg-gupta requested a review from gottesmm February 24, 2021 21:52
void printForwardingOwnershipKind(OwnershipForwardingMixin *inst,
SILValue op) {
if (inst->getForwardingOwnershipKind() != op.getOwnershipKind()) {
*this << " [" << inst->getForwardingOwnershipKind() << " forwarding] ";
Copy link
Contributor

@gottesmm gottesmm Feb 24, 2021

Choose a reason for hiding this comment

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

Can you use a syntax that is a postfix? That is:

unchecked_ref_cast %0 : $Foo to $Foo2, forwarding: @owned

I think with time this will make updating tests a lot easier.

createStructExtract(SILLocation Loc, SILValue Operand, VarDecl *Field,
SILType ResultTy,
ValueOwnershipKind forwardingOwnershipKind) {
return insert(new (getModule()) StructExtractInst(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little nervous about struct_extract/tuple_extract. I am worried this may let them take an owned value by mistake. My thought: add a check in SILVerifier itself since this is a sort of structural test on StructExtract, TupleExtract. That I am cool.

auto DerivativeFunctions =
JVPAndVJPFunctions.hasValue()
? ArrayRef<SILValue>(
reinterpret_cast<SILValue *>(JVPAndVJPFunctions.getPointer()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe and correct? Can this be done in a cleaner way?

Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

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

Talked with @meg-gupta

They are going to make the changes that I suggested (change the printing, add a verifier check for struct_extract, tuple_extract, and fix the derivative instruction code). Once that is done, this is ready to go/merge!

+1!

cloning

forwardingOwnershipKind can differ from the operand's ownershipKind. We
need to copy forwardingOwnershipKind while cloning these instructions.

Also print the forwarding ownership kind when it differs from its
operand's ownershipKind

This is a follow up of swiftlang#36063
@meg-gupta meg-gupta force-pushed the fixcloneforwardingownership branch from a605f18 to 5660643 Compare February 25, 2021 02:30
@meg-gupta
Copy link
Contributor Author

Thanks. I made all the suggested fixes.

@meg-gupta
Copy link
Contributor Author

@swift-ci test and merge

@swift-ci swift-ci merged commit b144527 into swiftlang:main Feb 25, 2021
@eeckstein
Copy link
Contributor

@meg-gupta When you change the printed SIL, please also make this parsable in SILParser and document it in SIL.rst

@meg-gupta
Copy link
Contributor Author

Thanks @eeckstein . Fixing in #36263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants