Skip to content

When converting load [copy] -> load_borrow, do not insert end_borrow if the block insert point is a dead end block. #34280

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Oct 12, 2020

This is important to do since otherwise, we may be implicitly reducing the
lifetime of a value which we can not do yet since we do not require all interior
pointer instructions to be guarded by borrows (yet). Once that constraint is in
place, we will not have this problem.

Consider a situation where one has a @owned switch_enum on an
indirect box case which is post-dominated by an unreachable that we want
to convert to @guaranteed:

  enum MyEnum {
    indirect case FirstCase(Int)
    ...
  }

  bb0(%in_guaranteed_addr : $*MyEnum):
    ...
    %0 = load [copy] %in_guaranteed_addr : $*MyEnum
    switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...

  bb1(%1 : @owned ${ var Int }):
    %2 = project_box %1 : ${ var Int }, 0
    %3 = load [trivial] %2 : $*Int
    apply %log(%3) : $@convention(thin) (Int) -> ()
    unreachable

In this case, we will not have a destroy_value on the box, but we may
have a project_box on the box. This is ok since we are going to leak the
value. But since we are using all consuming uses to determine the
lifetime, we will want to insert an end_borrow at the head of the
switch_enum dest block like follows:

  bb0(%in_guaranteed_addr : $*MyEnum):
    ...
    %0 = load_borrow %in_guaranteed_addr : $*MyEnum
    switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...

  bb1(%1 : @guaranteed ${ var Int }):
    end_borrow %1 : ${ var Int }
    %2 = project_box %1 : ${ var Int }, 0
    %3 = load [trivial] %2 : $*Int
    apply %log(%3) : $@convention(thin) (Int) -> ()
    unreachable

which would violate ownership invariants. Instead, we need to realize
that %1 is dominated by a dead end block so we may not have a
destroy_value upon it meaning we should just not insert the end_borrow
here. If we have a destroy_value upon it (since we did not get rid of a
destroy_value), then we will still get rid of the destroy_value if we are
going to optimize this, so we are still correct.

rdar://68096662

…if the block insert point is a dead end block.

This is important to do since otherwise, we may be implicitly reducing the
lifetime of a value which we can not do yet since we do not require all interior
pointer instructions to be guarded by borrows (yet). Once that constraint is in
place, we will not have this problem.

Consider a situation where one has a @owned switch_enum on an
indirect box case which is post-dominated by an unreachable that we want
to convert to @guaranteed:

  enum MyEnum {
    indirect case FirstCase(Int)
    ...
  }

  bb0(%in_guaranteed_addr : $*MyEnum):
    ...
    %0 = load [copy] %in_guaranteed_addr : $*MyEnum
    switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...

  bb1(%1 : @owned ${ var Int }):
    %2 = project_box %1 : ${ var Int }, 0
    %3 = load [trivial] %2 : $*Int
    apply %log(%3) : $@convention(thin) (Int) -> ()
    unreachable

In this case, we will not have a destroy_value on the box, but we may
have a project_box on the box. This is ok since we are going to leak the
value. But since we are using all consuming uses to determine the
lifetime, we will want to insert an end_borrow at the head of the
switch_enum dest block like follows:

  bb0(%in_guaranteed_addr : $*MyEnum):
    ...
    %0 = load_borrow %in_guaranteed_addr : $*MyEnum
    switch_enum %0 : $MyEnum, case #MyEnum.FirstCase: bb1, ...

  bb1(%1 : @guaranteed ${ var Int }):
    end_borrow %1 : ${ var Int }
    %2 = project_box %1 : ${ var Int }, 0
    %3 = load [trivial] %2 : $*Int
    apply %log(%3) : $@convention(thin) (Int) -> ()
    unreachable

which would violate ownership invariants. Instead, we need to realize
that %1 is dominated by a dead end block so we may not have a
destroy_value upon it meaning we should just not insert the end_borrow
here. If we have a destroy_value upon it (since we did not get rid of a
destroy_value), then we will still get rid of the destroy_value if we are
going to optimize this, so we are still correct.

rdar://68096662
@gottesmm gottesmm requested review from atrick and meg-gupta October 12, 2020 20:12
@gottesmm
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0e05b51

@gottesmm
Copy link
Contributor Author

@swift-ci test linux platform

@gottesmm
Copy link
Contributor Author

@swift-ci clean test linux platform

Copy link
Contributor

@meg-gupta meg-gupta left a comment

Choose a reason for hiding this comment

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

lgtm

@gottesmm gottesmm merged commit 36e95ca into swiftlang:main Oct 13, 2020
@gottesmm gottesmm deleted the pr-94bbbe81fdf8a4d1212dd582f8fad1c619207575 branch October 13, 2020 02:45
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.

3 participants