[ownership] Add simple support for concatenating together simple live ranges #30087
Conversation
|
@swift-ci benchmark |
|
@swift-ci smoke test |
| SILValue operand = cvi->getOperand(); | ||
| if (operand.getOwnershipKind() != ValueOwnershipKind::Owned) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This looks like an intentional style change toward using braces for single-line expression followed by single line statement, because I've seen it in the last few PRs. Or is it just a recurring accident?
(incidentally, I don't think the style needs to dictate this, just wondering if I should stop telling people how to do it in reviews)
There was a problem hiding this comment.
I prefer to always have braces in this case because I think I made a mistake b/c of that once when refactoring. So I just do it to avoid having to think about the problem.
There was a problem hiding this comment.
I just told @meg-gupta not to do this because I was trying to be a good reviewer, so I have to take it back now. Frankly, I think it's fine to use braces whenever you want to.
| // DISCUSSION: The reason why we do not need to check if the destroy_value is | ||
| // after the copy_value in the block is that if the destroy_value of the | ||
| // copy_value's operand was before the copy_value, the copy_value would be a | ||
| // use-after-free of the operand. |
There was a problem hiding this comment.
Good discussion, but the implementation looks incorrect because it appears to be ignoring non-consuming uses of the copy's source after the copy's destroy.
There was a problem hiding this comment.
Yes. The ownership verifier caught it. I just tracked it down.
gottesmm
force-pushed
the
pr-7d5f0d45e4677ebbd92aeb1e261ab388a4ca5756
branch
from
3d13b7a to
c43991b
Compare
|
@swift-ci smoke test |
|
@swift-ci benchmark |
|
@swift-ci smoke test |
|
@swift-ci benchmark |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci benchmark |
|
And also I forgot about fixing one other thing to make this work a little better. |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
gottesmm
force-pushed
the
pr-7d5f0d45e4677ebbd92aeb1e261ab388a4ca5756
branch
from
c43991b to
fc348e2
Compare
|
@swift-ci smoke test |
|
@swift-ci benchmark |
|
@atrick I added two more cases here that I think will be useful:
I have seen variations on that in various frameworks. |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
@swift-ci benchmark |
|
@Catfish-Man some juice maybe for string comparison. |
|
... opaque values opt is failing. Optimizing it too much. |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -Onone
Code size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Going to do a check that the few -O regressions are real or not. I have a feeling they are not, but I am going to check. (Pretty small). |
|
Iterate data is real, but it is due to an inlining change. Specifically, we inline sequence data in one case and not in the other. |
|
Out of curiosity, seeing if the improvements are real or not. |
… ranges that do not need LiveRange analysis. Specifically, this PR adds support for optimizing simple cases where we do not need to compute LiveRanges with the idea of first doing simple transforms that involve small numbers of instructions first. With that in mind, we only optimize cases where our copy_value has a single consuming user and our owned value has a single destroy_value. To understand the transform here, consider the following SIL: ``` %0 = ... %1 = copy_value %0 (1) apply %guaranteedUser(%0) (2) destroy_value %0 (3) apply %cviConsumer(%1) (4) ``` We want to eliminate (2) and (3), effectively joining the lifetimes of %0 and %1, transforming the code to: ``` %0 = ... apply %guaranteedUser(%0) (2) apply %cviConsumer(%0) (4) ``` Easily, we can always do this transform in this case since we know that %0's lifetime ends strictly before the end of %1's due to (3) being before (4). This means that any uses that require liveness of %0 must be before (4) and thus no use-after-frees can result from removing (3) since we are not shrinking the underlying object's lifetime. Lets consider a different case where (3) and (4) are swapped. ``` %0 = ... %1 = copy_value %0 (1) apply %guaranteedUser(%0) (2) apply %cviConsumer(%1) (4) destroy_value %0 (3) ``` In this case, since there aren't any liveness requiring uses of %0 in between (4) and (3), we can still perform our transform. But what if there was a liveness requiring user in between (4) and (3). To analyze this, lets swap (2) and (4), yielding: ``` %0 = ... %1 = copy_value %0 (1) apply %cviConsumer(%1) (4) apply %guaranteedUser(%0) (2) destroy_value %0 (3) ``` In this case, if we were to perform our transform, we would get a use-after-free due do the transform shrinking the lifetime of the underlying object here from ending at (3) to ending at (4): ``` %0 = ... apply %cviConsumer(%1) (4) apply %guaranteedUser(%0) (2) // *kaboom* ``` So clearly, if (3) is after (4), clearly, we need to know that there aren't any liveness requiring uses in between them to be able to perform this optimization. But is this enough? Turns out no. There are two further issues that we must consider: 1. If (4) is forwards owned ownership, it is not truly "consuming" the underlying value in the sense of actually destroying the underlying value. This can be worked with by using the LiveRange abstraction. That being said, this PR is meant to contain simple transforms that do not need to use LiveRange. So, we bail if we see a forwarding instruction. 2. At the current time, we may not be able to find all normal uses since all of the instructions that are interior pointer constructs (e.x.: project_box) have not been required yet to always be guarded by borrows (the eventual end state). Thus we can not shrink lifetimes in general safely until that piece of work is done. Given all of those constraints, we only handle cases here where (3) is strictly before (4) so we know 100% we are not shrinking any lifetimes. This effectively is causing our correctness to rely on SILGen properly scoping lifetimes. Once all interior pointers are properly guarded, we will be able to be more aggressive here. With that in mind, we perform this transform given the following conditions noting that this pattern often times comes up around return values: 1. If the consuming user is a return inst. In such a case, we know that the destroy_value must be before the actual return inst. 2. If the consuming user is in the exit block and the destroy_value is not. 3. If the consuming user and destroy_value are in the same block and the consuming user is strictly later in that block than the destroy_value. In all of these cases, we are able to optimize without the need for LiveRanges. I am going to add support for this in a subsequent commit.
gottesmm
force-pushed
the
pr-7d5f0d45e4677ebbd92aeb1e261ab388a4ca5756
branch
from
fc348e2 to
bdf6143
Compare
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
|
@swift-ci smoke test |
Specifically, I added support for handling cases where the first live range ends
in the same block as the second live range starts, but after the start of that
live range. As an example:
==>
Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.