[6.0][region-isolation] Reuse ActorInstance::lookThroughInsts when computing the actor instance for SILIsolationInfo purposes. #75044
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
Given a function or a partial_apply with an isolated parameter, we do not know immediately what the actual isolation is of the function or partial_apply since we do not know which instance will be applied to the function or partial_apply. In this commit, I introduce a new bit into SILIsolationInfo that tracks this information upon construction and allows for it to merge with ownership that has the appropriate type and a specific instance. Since the values that created the two isolations, will be in the same region this should ensure that the value is only ever in a flow sensitive manner in a region with only one actor instance (since regions with isolations with differing actor instances are illegal). (cherry picked from commit 0c25480)
…ng routines for SILIsolationInfo. Before we wouldn't print them in all situations and even more so a few of the printing routines did not have it at all. This just adds a centralized SILIsolationInfo::dumpOptions() method and then goes through all of the printing helpers and changes them to use them as appropriate. (cherry picked from commit 6129f4e)
…ng the actor instance for SILIsolationInfo purposes.
We are already using this routine in other parts of TransferNonSendable to
ensure that we look through common insts that SILGen inserts that do not change
the actual underlying actor instance that we are using. In this case, I added
support for casts, optional formation, optional extraction, existential ref
initialization.
As an example of where this came up is the following test case where we fail to
look through an init_existential_ref.
```swift
public actor MyActor {
private var intDict: [Int: Int] = [:]
public func test() async {
await withTaskGroup(of: Void.self) { taskGroup in
for (_, _) in intDict {}
await taskGroup.waitForAll() // Isolation merge failure happens here
}
}
}
```
I also added the ability to at the SIL level actual test out this merge
condition using the analysis test runner. I used this to validate that this
functionality works as expected in a precise way.
rdar://130113744
(cherry picked from commit 62c6de7)
|
@swift-ci test |
DougGregor
approved these changes
Jul 8, 2024
DougGregor
reviewed
Jul 8, 2024
| } | ||
| } | ||
|
|
||
| // Look Through wrapping in an enum. |
There was a problem hiding this comment.
Minor nit: look through wrapping in an optional
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.
Explanation: This PR improves our ability to properly unify actor instances by standardizing region isolation to use the same ActorInstance::lookThroughInsts in both RegionAnalysis and SILIsolationInfo inference. The result is that cases like the following compile since we are able to ensure that all of the #isolation parameters are viewed as having the same isolation as the ambient isolation in callers:
Before we would view them as different and upon isolation merges would hit merge failures (since a region cannot have two values within the region that are actor isolated to different actors). This merge failure would then cause us to emit a "pattern compiler doesn't understand" error at taskGroup.waitForAll().
Radars:
Original PRs:
Risk: Low. This allows the compiler to view more actor instances as the same in a very conservative way since we just look through instructions that propagate the same actor identity (e.x.: copy_value, move_value, init_existential_ref, optional formation). If looking through these instructions caused any issue, it would show a differing significant issue in the compiler that would be a separate error.
Testing: Added/Ran tests
Reviewer: N/A