Skip to content

[OwnershipUtils] Classify moves from limited-use values as redundant. #64448

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 1 commit into from
Mar 20, 2023

Conversation

nate-chandler
Copy link
Contributor

When a move separates a non-escaping lifetime from an escaping lifetime, it is still redundant if the original lifetime couldn't be optimized because it's already as small as possible.

Moves from limited use values are redundant.  When a move separates a
non-escaping lifetime from an escaping lifetime, it is still redundant
if the original lifetime couldn't be optimized because it's already as
small as possible.
@nate-chandler nate-chandler changed the title [OwnershipUtils] Classify moves from limited-use moves as redundant. [OwnershipUtils] Classify moves from limited-use values as redundant. Mar 17, 2023
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@atrick
Copy link
Contributor

atrick commented Mar 20, 2023

Is this overly conservative? The original value's lifetime won't be affected if the move is the only consuming use. We don't care about non-consuming uses.

@nate-chandler
Copy link
Contributor Author

nate-chandler commented Mar 20, 2023

@atrick

The original value's lifetime won't be affected if the move is the only consuming use.

That's true--the moved-to value's lifetime, however, would be affected if the moved-from value escaped. We don't want to introduce escapes into the moved-to value's lifetime if it doesn't already escape.

This patch does a cheap check (original is not a phi and its only user is the move) that the original doesn't escape and returns early that the move_value is redundant if it doesn't. Afterwards, it compares the escaping of the original and the moved-to value. If the original doesn't escape, it checks only whether the move is the only consuming use (now that the original value is known not to escape unlike before).

Am I missing something?

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

A couple comments, possibly for a subsequent PR.

auto originalHasEscape = hasPointerEscape(original);

// (3) Escaping matches? (Expensive check, saved for last.)
if (moveHasEscape != originalHasEscape) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be more clear:

if (moveHasEscape == originalHasEscape)
  return true

if (hasPointerEscape(mvi) != hasPointerEscape(original)) {
// The move doesn't alter constraints: ownership and lexicality match.

auto *singleUser =
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments are great. But I still somehow missed that that singleUser check is actually part of condition #3, and only a way to short-circuit the complete, but slower, logic below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up at #64483 .

@nate-chandler
Copy link
Contributor Author

Will post a follow-up to address those two.

@nate-chandler nate-chandler merged commit f5e8fc2 into swiftlang:main Mar 20, 2023
@nate-chandler nate-chandler deleted the eliminate-more-moves branch March 20, 2023 18:31
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.

2 participants