[CanonicalizeOSSALifetime] Extend Onone lifetimes. #61464
Conversation
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
from
8a6eefe to
2decfcb
Compare
Previously, the destroys set (now set vector) wasn't ever being cleared. The result was that users could get overly pessimistic behavior if they had previously used the utility with a destroy that came after the destroys relevant for its current run. Here, it is cleared when the utility is initialized with a new def. Addresses a TODO in the copy_propagation test.
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
from
2decfcb to
b840cb3
Compare
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
from
b840cb3 to
377ec0d
Compare
There was a problem hiding this comment.
This is awesome. It's such a relief to see all the nasty stuff that accumulated because of poison values and debug support stripped away and have everything composed on top of our incremental liveness utility. This code is really clear, solid, and maintainable now
| } else { | ||
| for (auto *grandPredecessor : predecessor->getPredecessorBlocks()) { | ||
| liveness.updateForUse(grandPredecessor->getTerminator(), | ||
| /*lifetimeEnding*/ false); |
There was a problem hiding this comment.
Can you try out a test case where predecessor is itself a control-flow merge? We currently assume that a phi operand (branch instruction) can never be the last use unless it is lifetime-ending (This needs to be more clearly commented and asserted somewhere in PrunedLiveness). Breaking this assumption probably means we end up visiting the same insertion point multiple times.
One option is to simply insert the destroy_value at the correct place here and make it the live use.
Another option is to add a special case to visitInsertionPoints to handle nonconsuming-last-use-phis with extra book-keeping.
There was a problem hiding this comment.
Oops, it looks like all the tests I added didn't make it into the PR. Fixed that (test/SILOptimizer/copy_propagation_onone.sil). Not to say that this issue is resolved, just that the issue of all new tests missing is.
There was a problem hiding this comment.
Added. It exposed an issue when attempting to extend a boundary along multiple cfg edges with the same destination.
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
6 times, most recently
from
25b99ee to
86baa4a
Compare
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
from
86baa4a to
f47757a
Compare
It is possible for phis to be marked live. With guaranteed phis, they will be the last uses and be non-consuming. In this case, the merge block will have multiple predecessors whose terminators are on the boundary. When inserting destroys, track whether a merge point has been visited previously. To facilitate this, restructure the boundary extension and destroy insertion code. Previously, the extender was building up a list of places at which to insert destroys. In particular it was using the "boundary edge" collection for all blocks at the beginning of which a destroy should be created. In particular, it would add merge blocks. Such blocks are not boundary blocks. Here, the extender produces a PrunedLivenessBoundary which doesn't violate that invariant. This required some changes to the destroy insertion code to find where to insert destroys. It is now similar to PrunedLivenessBoundary::visitInsertionPoints and could be used as a template for a PrunedLivenessBoundary::visitBoundaryPoints through which ::visitInsertionPoints might be factored.
Rather than having finding the boundary be a single combined step, separate finding the original boundary from extending that boundary. This enables inserting an optional step between those steps, namely to extend unconsumed liveness to its original extent at Onone.
To improve the debugging experience of values whose lifetimes are canonicalized without compromising the semantics expressed in the source language, when canonicalizing OSSA lifetimes at Onone, lengthen lifetimes as much as possible without incurring copies that would be eliminated at O. rdar://99618502
nate-chandler
force-pushed
the
branch/rdar99618502/OnoneLifetimes
branch
from
f47757a to
03253db
Compare
|
@swift-ci please test and merge |
To improve the debugging experience of values whose lifetimes are canonicalized without compromising the semantics expressed in the source language, when canonicalizing OSSA lifetimes at Onone, lengthen lifetimes as much as possible without incurring copies that would be eliminated at O.
rdar://99618502