[LifetimeCompletion] Mark lifetimes in dead-end loops. #73859
Conversation
nate-chandler
force-pushed
the
lifetime-completion/20240523/1
branch
2 times, most recently
from
f7cc892 to
92eaad0
Compare
|
@swift-ci please test |
|
@swift-ci please test windows platform |
lib/SIL/Utils/OwnershipLiveness.cpp
Outdated
| @@ -38,6 +38,13 @@ struct LinearLivenessVisitor : | |||
| linearLiveness(linearLiveness){} | |||
|
|
|||
| bool handleUsePoint(Operand *use, UseLifetimeConstraint useConstraint) { | |||
| if (isa<ExtendLifetimeInst>(use->getUser())) { | |||
There was a problem hiding this comment.
I'm confused about the handleUsePoint contract.
If extend_liveness should be passed to handleUsePoint as LifetimeEnding, then that should be specified in the function interface comment (similar to the comment in the commit log) since that is an inversion of the expected value. Then we should not need additional checks within the visitor implementations of handleUsePoint.
Preferably, extend_liveness would be passed to handleUsePoint as NonLifetimeEnding to avoid confusion, and the visitor implementations of handleUsePoint will check for extend_liveness if they need to invert the value. In that case, the explanatory comment in the commit message needs to be on each visitor implementation that inverts the sense of extend_lifetime.
Instead, this diff shows extra checks for extend_lifetime on both sides of handleUsePoint and there aren't any comments to explain away the confusion.
There was a problem hiding this comment.
That makes sense. Fixed to pass NonLifetimeEnding to handleUsePoint. And actually, is there any reason that we want the extend_lifetime instructions to be added to PrunedLiveness with /*lifetimeEnding=*/true? I haven't thought of one--we still get all users within the LinearLifetime boundary either way--so I removed that special behavior.
There was a problem hiding this comment.
is there any reason that we want the extend_lifetime instructions to be added to PrunedLiveness with /lifetimeEnding=/true? I haven't thought of one--we still get all users within the LinearLifetime boundary either way--so I removed that special behavior.
Oh, wonderful. I just assumed that you needed to invert the lifetimeEnding value in PrunedLiveness to avoid special cases elsewhere. It's always better if the code doesn't need to lie about anything though.
nate-chandler
force-pushed
the
lifetime-completion/20240523/1
branch
3 times, most recently
from
6499dc0 to
9107952
Compare
|
@swift-ci please test |
| @@ -245,6 +259,53 @@ void State::initializeAllNonConsumingUses( | |||
| // Consuming Use Initialization | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| void State::initializeAllExtendLifetimeUses( | |||
| ArrayRef<Operand *> linearLifetimeEndingUses, | |||
There was a problem hiding this comment.
Could this parameter be extendLifetimeUses to make things clear?
| @@ -389,6 +392,14 @@ void OSSALifetimeCompletion::visitAvailabilityBoundary( | |||
| visitor.propagateAvailablity(result); | |||
|
|
|||
| visitor.visitAvailabilityBoundary(result, visit); | |||
|
|
|||
| visitUsersOutsideLinearLivenessBoundary( | |||
There was a problem hiding this comment.
It looks like this makes AvailabilityBoundaryVisitor a four step process. Its class-level comment is no longer replete.
There was a problem hiding this comment.
Added the new step to the class-level comment. It does feel now (after this PR, not after expanding the comment) like the factoring/naming here is not quite ideal (there's the availability boundary and then there are these infinite loop instructions), but we can tweak that later on.
There was a problem hiding this comment.
Clarified the top-level comment to indicate that AvailabilityBoundaryVisitor only does half of OSSALifetimeCompletion::visitAvailabilityBoundary. Also gave the class only a single public member function.
nate-chandler
force-pushed
the
lifetime-completion/20240523/1
branch
2 times, most recently
from
c380bab to
12589b6
Compare
|
@swift-ci please test |
Clarify methods. Unfortunately, without other changes I haven't identified, the `enum class` can't be made an `enum` for ideal usage.
It visits the availability boundary, so call it `visitAvailabilityBoundary`. It's not clear what "unreachable lifetime ends" are.
It indicates that the value's lifetime continues to at least this point. The boundary formed by all consuming uses together with these instructions will encompass all uses of the value.
When visiting consumes, also visit `extend_lifetime` instructions. These instructions are not lifetime ending, but together with the consumes, they enclose the users of a value. Add a flag to LinearLiveness to control whether these instructions are added so that the verifier can use verify that all such instructions appear outside the linear lifetime boundary (not including them).
When visiting an availability boundary, note what kind of end is involved. For now, there's only one.
VisitAvailabilityBoundary now has a single public method.
It visits users discovered by an SSAPrunedLiveness instance (such as that used by InteriorLiveness) which are outside the LinearLifetime boundary.
The new instructions are inserted after every "user" (according to InteriorLiveness' SSAPrunedLiveness instance) outside the linear liveness boundary.
nate-chandler
force-pushed
the
lifetime-completion/20240523/1
branch
from
12589b6 to
84da0ed
Compare
|
@swift-ci please smoke test |
The new instructions are inserted after every "user" (according to
InteriorLiveness'SSAPrunedLivenessinstance) outside the linear liveness boundary.