[move-only] Batched resilient changes #65842
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
… an address argument instead of an object. The signature was already correct, just the logic in the deinit code assumed that we would always have an object. rdar://109170069
… resilient guaranteed arguments. The only difference from normal concrete guaranteed parameter emission is we have a load_borrow on the argument before the copy_value. That is instead of: ``` bb0(%0 : @guaranteed $Type): %1 = copy_value %0 %2 = mark_must_check [no_consume_or_assign] %1 ``` we have, ``` bb0(%0 : $*Type): // in_guaranteed %1 = load_borrow %0 %2 = copy_value %1 %3 = mark_must_check [no_consume_or_assign] %2 ``` So I just needed to update the checker to recognize that pattern. This is tested by just making sure that the checker can handle the borrowVal in the tests without emitting an error. rdar://109170906
…ow if we call a resilient function that takes it in_guaranteed.
This ensures that given a class that contains a noncopyable type that contains
another noncopyable type:
```
@_moveOnly struct S2 {}
@_moveOnly struct S { var s2: S2 }
class C { var s: S }
```
if we call a resilient function that takes C.S.S2:
```
borrowVal(c.s.s2)
```
we properly spill s2 onto the stack using a store_borrow.
Why Do This?
------------
Currently SILGenLValue treats ref_element_addr as a base that it needs to load
from for both copyable and non-copyable types. We keep a separation of concerns
and require emission of resilient functions to handle these loaded values. For
copyable types this means copying the value and storing it into a temporary
stack allocation. For noncopyable types, we never actually implemented this so
we would hit an error in SILGenApply telling us that our resilient function
expected an address argument, but we are passing an object.
To work around this, I updated how we emit borrowed lvalue arguments to in this
case to spill the value into a temporary allocation using a store_borrow. I also
included a test that validates that we properly have a read exclusivity scope
around the original loaded from memory for the entire call site so even though
we are performing a load_borrow and then spilling it, we still have read
exclusivity to the original memory for the entire region meaning that we still
preserve the semantics.
rdar://109171001
|
@swift-ci smoke test |
I hit a weird bug where for some reason it wasn't working. I can't reproduce it now, so it makes sense to use the correct syntax.
|
@swift-ci smoke test |
jckarter
approved these changes
May 10, 2023
…e_borrow if we are using lowered addresses. When opaque values are enabled, we do not need this store_borrow. This was caught by the following tests: Swift(macosx-x86_64) :: SILGen/opaque_values_silgen.swift Swift(macosx-x86_64) :: SILGen/opaque_values_silgen_resilient.swift
|
@swift-ci smoke test |
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.
In this PR, I include 3 small batched changes that fix some of the issues we are seeing with resilience. Specifically: