[5.9][move-only] Address Only Batched Fixes #65645
Merged
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
Specifically, I changed emitRValueForDecl and SILGenProlog to do the right thing. I also added some tests. Some notes: 1. We currently consider using a copyable field of a move only address type to be a consume of that type. I am going to fix that in the next commit to make it easier to understand. 2. I am going to need to write more tests/flesh out the behavior more. I am sure there is more here. rdar://105106470 (cherry picked from commit 20958c9)
…ess only type as consuming the address only type. rdar://105106470 (cherry picked from commit d10fdf7)
(cherry picked from commit 6a7dc26)
(cherry picked from commit a0a3aec)
…only type used by escaping closures. When we have a loadable type, SILGen always eagerly loads the value, so the move checker handled checking those cases by just looking for loads and emitting the error based off of a load. Of course for address only types, the codegen looks slightly different (we consume the value directly rather than load it). So this change just teaches the checker how to do that. rdar://105106470 (cherry picked from commit be7667b)
This just creates a new SILUndef with the same type as the SILValue. This just prevents one from having to write SILUndef::get(value->getType(), *value->getModule()). (cherry picked from commit 92116ba)
It was actually testing that we properly handle something related to debug info. With subsequent changes, this causes a crash since it doesn't match the expected output. (cherry picked from commit d10573a)
…nd fields of an address only move only type correctly. (cherry picked from commit f056c05)
…cts. (cherry picked from commit e4cdc66)
… form. For some reason in a previous commit, this started to work so I just added support. In this PR, it changed again and the test started to fail again. We are not shipping noimplicitcopy in 5.9, so I just changed the error message to the old "I don't understand" error message. I additionally looked at why the failure is occuring. The reason why is that the frontend emits an unchecked_ref_cast since currently the moveonlywrapper_to_copyable instruction does not accept addresses (even though it could) and the field sensitive liveness analysis does not know how to represent an unchecked_ref_cast (which is ok, since we would like it to be rejected). (cherry picked from commit d25cd0b)
Contributor
Author
|
@swift-ci test |
…gument This happens for address-only move-only types. (cherry picked from commit c4ae7fe)
Contributor
Author
|
@swift-ci test |
jckarter
approved these changes
May 4, 2023
tbkka
approved these changes
May 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
CCC. This PR contains a few small commits that do some small cleanups and add
more tests/test coverage , so I did not include them in the CCC. The CCC for the
major changes are as follows:
Commit c1aa281:
• Description: This commit builds on address only support that is already in the
compiler but fixes a few places we were not handling things
correctly. Specifically, we were not handling address only noncopyable
function arguments correctly and we were inserted redundant mark_must_check
instructions when we were loading a noncopyable argument as an lvalue.
• Risk: Low risk. Only affects noncopyable types.
• Original PR: #65604
• Reviewed By: @jckarter
• Testing: I added diagnostic tests that checked this behavior.
• Resolves: rdar://108510644
Commit a8fea65:
• Description: The move checker was treating accesses to copyable fields of
noncopyable types as if the field was noncopyable. This change just causes us
to treat such uses as liveness requiring uses (meaning the original value must
be alive) but not as a consuming use.
• Risk: Low risk. Only affects noncopyable types.
• Original PR: #65604
• Reviewed By: @jckarter
• Testing: I updated tests that expected this behavior to no longer expect this
behavior (they expected this behavior to just memorialize the state of the
tree at that time and provide a test case when this was fixed).
• Resolves: rdar://108510987
Commit 9749a60
• Description: Teach the move checker how to correctly check an address only
type used by escaping closures. When we have a loadable type, SILGen always
eagerly loads the value, so the move checker handled checking those cases by
just looking for loads and emitting the error based off of a load. Of course
for address only types, the codegen looks slightly different (we consume the
value directly rather than load it). So this change just teaches the checker
how to handle those cases.
• Risk: Low. Only affects noncopyable types.
• Original PR: #65604
• Reviewed By: @jckarter
• Testing: I added both a SIL test case and a Swift diagnostic test case that
shows that we handle this behavior correctly.
• Resolves: rdar://108511866
Commit f52efde:
• Description: This is a commit on main that was commited as a separate PR that
is required to prevent test cases in this change from not crashing. It was not
originally cherry-picked to 5.9 since its original PR (adding drop_deinit)
needed additional work. I cherry-picked it into this PR since it was needed
for the rest of the code (which was commited as part of a later PR), work
successfully.
• Risk: Low. Only affects noncopyable types.
• Original PR: #65060
• Reviewed By: @jckarter
• Testing: Without this, as shown during testing on the CCC PR, we emit
incorrect code. The tests in tree already provide coverage for this.
• Resolves: rdar://108511534