[6.0][region-isolation] Fix handling of coroutine apply results. #75596
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
(cherry picked from commit c1ddeb2)
In this part of the code, we are attempting to merge all of the operands into the same region and then assigning all non-Sendable results of the function to that same region. The problem that was occuring here was a thinko due to the control flow of the code here not separating nicely the case of whether or not we had operands or not. Previously this did not matter, since we just used the first result in such a case... but since we changed to assign to the first operand element in some cases, it matters now. To fix this, I split the confused logic into two different easy to follow control paths... one if we have operands and one where we do not have an operand. In the case where we have a first operand, we merge our elements into its region. If we do not have any operands, then we just perform one large region assign fresh. This was not exposed by code that used non-coroutines since in SIL only coroutines today have multiple results. rdar://132767643 (cherry picked from commit 541863d)
|
@swift-ci test |
|
Original: #75590 |
|
@swift-ci test |
hborla
approved these changes
Aug 1, 2024
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: [region-isolation] Fix handling of coroutine apply results.
In this part of the code, we are attempting to merge all of the operands into
the same region and then assigning all non-Sendable results of the function to
that same region. The problem that was occuring here was a thinko due to the
control flow of the code here not separating nicely the case of whether or not
we had operands or not. Previously this did not matter, since we just used the
first result in such a case... but since we changed to assign to the first
operand element in some cases, it matters now. To fix this, I split the confused
logic into two different easy to follow control paths... one if we have operands
and one where we do not have an operand. In the case where we have a first
operand, we merge our elements into its region. If we do not have any operands,
then we just perform one large region assign fresh.
This was not exposed by code that used non-coroutines since in SIL only
coroutines today have multiple results.
Without this change, in such a cases, we will crash.
Radars:
Original PRs:
Risk: Low. What this change, we only change the behavior specifically for apply like things that have multiple results. Today the only instruction with this property begin_apply, so that is the only case that we could break by accepting this change (that is we could only break the case that is already broken).