[no-implicit-copy] Update SILGen/move checker to work with new patterns from copyable_to_moveonly and friends. #59611
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci test |
…ns from copyable_to_moveonly and friends. This involved doing the following: 1. Update the move only checker to look for new patterns. 2. Teach emitSemanticStore to use a moveonlywrapper_to_copyable to store moveonly values into memory. The various checkers will validate that this code is correct. 3. When emitting an apply, always unwrap move only variables. In the future, I am going to avoid this if a parameter is explicitly marked as also being moveonly (e.x.: @moveOnly parameter or @NoEscape argument). 4. Convert from moveOnly -> copyable on return inst automatically in SILGen. 5. Fix SILGenLValue emission so we emit an error diagnostic later rather than crash. This is needed to keep SILGen emitting move only addresses (that is no implicit copy address only lets) in a form that the move only checker then will error upon. Without this change, SILGen crashes instead of emitting an error diagnostic in the following test: .//test/SILOptimizer/move_only_checker_addressonly_fail.swift
gottesmm
force-pushed
the
moveonly-rebase
branch
from
882de75 to
22b77bb
Compare
|
@jckarter I forgot to upload the trivial type eliminator. I added that in the 2nd commit I added here and also added a small fix that I discovered when writing some Interpreter tests for this. |
|
@swift-ci test |
gottesmm
force-pushed
the
moveonly-rebase
branch
from
22b77bb to
b9a8101
Compare
|
@swift-ci test |
…ter move only checking. This pass lowers moveonly-ness from the IR after we have finished move only checking. The transform can be configured in two ways: such that it only handles trivial types and such that it does trivial and non-trivial types. For ease of use, I created two top level transforms (TrivialMoveOnlyTypeEliminator and MoveOnlyTypeElimintor) that invoke the two behaviors by configuring the underlying transform slightly differently. For now, I am running first the trivial-only and then the all of the above lowering. The trivial only pass will remain at this part of the pipeline forever, but with time we are going to move the lower everything pass later into the pipeline once I have audited the optimizer pipeline to just not perform any work on move only types. That being said, currently we do not have this guarantee and this patch at least improves the world and lets us codegen no implicit copy code again.
…es before materializing for a function argument. I discovered this as I began to write some Interpreter tests for move only. This was triggered by SILGen attempting to pass a non-trivial type to StringBuilder which is generic, so we needed to materialize. I also discovered a small bug where we were not properly ignoring class_method in the move only type eliminator. I just folded the small fix + a test for that into this commit.
gottesmm
force-pushed
the
moveonly-rebase
branch
from
b9a8101 to
3088fe1
Compare
|
@swift-ci test |
|
@swift-ci test macOS platform |
1 similar comment
|
@swift-ci test macOS platform |
jckarter
reviewed
Jun 27, 2022
|
|
||
| public func aggGenericStructAccessField<T>(_ x: AggGenericStruct<T>) { | ||
| @_noImplicitCopy let x2 = x // expected-error {{'x2' consumed more than once}} | ||
| print(x2.lhs) // expected-note {{consuming use}} |
There was a problem hiding this comment.
Projecting a stored property, or more generally, calling a get or read accessor with a guaranteed base, should not be a consuming use.
There was a problem hiding this comment.
The reason this is a consuming use is b/c of print. Print consumes values unfortunately due to the way we construct the array.
jckarter
reviewed
Jun 27, 2022
| ///////////////////////////////// | ||
|
|
||
| public func klassNoImplicitCopyArgument(@_noImplicitCopy _ x: Trivial) -> Trivial { | ||
| return x |
There was a problem hiding this comment.
For a nontrivial x, returning it here would be considered a copy, unless the argument were __owned. Should this also be flagged as a consuming use?
There was a problem hiding this comment.
I actually have @_noImplicitCopy arguments being +1 currently. I am looking at changing that (I can do it in this PR)
|
@jckarter Hows this sound: can we get in this PR and then in follow on PRs I will do the following:
|
|
That works for me. |
jckarter
approved these changes
Jun 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This involved doing the following:
Update the move only checker to look for new patterns.
Teach emitSemanticStore to use a moveonlywrapper_to_copyable to store moveonly
values into memory. The various checkers will validate that this code is
correct.
When emitting an apply, always unwrap move only variables. In the
future, I am going to avoid this if a parameter is explicitly marked as also
being moveonly (e.x.: @moveOnly parameter or @NoEscape argument).
Convert from moveOnly -> copyable on return inst automatically in SILGen.
Fix SILGenLValue emission so we emit an error diagnostic later rather than
crash. This is needed to keep SILGen emitting move only addresses (that is no
implicit copy address only lets) in a form that the move only checker then
will error upon. Without this change, SILGen crashes instead of emitting an
error diagnostic in the following test:
.//test/SILOptimizer/move_only_checker_addressonly_fail.swift
Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.