Skip to content

[move-only] Add support for switch_enum in borrow2destructure #63530

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 17 commits into from
Feb 9, 2023

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Feb 8, 2023

Previously for move only types, SILGen would emit an owned switch_enum. This became an issue since for objects, SILGenPattern always attempts to push it towards borrowing, so eventually we would borrow which could have introduced an inadvertent copy along the decision tree before bindings are assigned.

This patch fixes this issue by doing the following:

  1. Refactoring Borrow2Destructure so that it can be used for switch_enum arguments and what it already does.
  2. Integrates Borrow2Destructure into the address checker so that when we switch_enum on loaded values, we do not run into this problem.
  3. Moves the verification that we haven't introduced unfortunate copies from the object checker into the address checker and then moved the object checker before the address checker in the pipeline.
  4. Added to the address checker validation that we never after it runs have load [copy]/copy_addr [!take] on move only types in addition to the copy_value change.
  5. It splits out PrunedLiveBlocks and FieldSensitivePrunedLiveBlocks so that they use two different implementations. I tried to split this out of a larger patch in a different PR, but this patch began to rely on it so I moved it into this PR. I am going to close the other one.

…n of PrunedLiveBlocks called FieldSensitivePrunedLiveBlocks.

This will let the non-field sensitive version use a more performant
implementation internally. This is important since PrunedLiveBlocks is used in
the hot path when working with Ownership SSA, while the field sensitive version
is only used for certain diagnostics.

NOTE: I did not refactor PrunedLiveness to use the faster implementation... this
is just a quick pass over the code to prepare for that change.
… invoked by the tester/main checker pass rather than a utility.

This is in preparation for sinking the main computation of the pass into an
implementation local helper struct that can process multiple values. I am doing
this so we can use the same code to process switch_enum arguments and am using
the current implementation to make sure I don't break anything.
…ansform into a private namespace in preparation for sinking into the implementation file.
… its own header and out of MoveOnlyObjectChecker.h
… its interface by moving gatherUses onto the impl.
…DestructureTransform driver to the local Implementation.
… and allow for it to be initialized separately from construction.
…ecific cleanup and inserting compensating destroys.
…terestingOperandIdnexMap, and blocksToUses into the implementation.

For now, I am creating new implementations every time... but once I get the
tests passing, I am going to improve the performance by reusing the same
Implementation for all computations so we can reuse memory.
As mentioned previously, I am right now creating new Implementations for each
switch_enum argument. Once I get tests working I am going to refactor and reuse
the same implementation so I can reuse memory.

NOTE: This currently does not impact SILGen since I did not change SILGenPattern
to emit noncopyable switched upon values as guaranteed. Keep in mind that I am
not going to do this for no implicit copy though since no implicit copy values
cannot contain move only values so we do not need to work with destructures.
…r in the pass pipeline and move post checker verification into the MoveOnlyAddressChecker.

The reason why I am doing this is that:

1. There are sometimes copy_value on move only values loaded from memory that
the MoveOnlyAddressChecker needs to eliminate.

2. Previously, the move only address checker did not rewrite copy_value ->
explicit_copy_value if it failed in diagnostics (it did handle load [copy] and
copy_addr though). This could then cause the copy_value elimination verification
in the MoveOnlyObjectChecker to then fail. So this suggested that I needed to
move the verification from the object checkt to the address checker.

3. If we run the verification in the address checker, then the object checker
(which previously ran before the address checker) naturally needed to run
/before/ the address checker.
…dle switch_enum, emit move only enum switches at +0.
This test was taken from some generated SIL when the address checker ran
/before/ the object checker and before the address checker validated that the
object checker also didn't leave any copy_value. This is just cleaning up the
source as if the object checker ran on this specific function so the address
checker check doesn't trip.
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 8, 2023

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 8, 2023

@swift-ci smoke test macOS platform

@gottesmm gottesmm merged commit 97b68be into swiftlang:main Feb 9, 2023
@gottesmm gottesmm deleted the moveonly-enum-destructure branch February 9, 2023 08:27
@gottesmm
Copy link
Contributor Author

This was rdar://104874356

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.

1 participant