Repository navigation
Conversation
|
LLM disclosure: LLMs were used to double-check the correctness of the analysis against the RFC, Miri and existing MIR passes. It was useful since it found several issues, notably around the behavior of call destination places in the unwind path. |
e7ac990 to
191a6f0
Compare
191a6f0 to
8f7b938
Compare
8f7b938 to
1c34f63
Compare
This comment has been minimized.
This comment has been minimized.
|
Would it be possible to test this independently from move elimination? |
|
We don't really have good infrastructure for testing analysis passes directly. The best I can do is generate some analysis snapshots, but it won't have any CHECK comments since those only look at the final MIR. My preference is to instead have this tested indirectly through the tests in MoveElimination. |
|
Perhaps we could annotate final MIR with dataflow results and run file check on it? For example, along the lines of https://github.com/tmiasko/rust/tree/pretty-dataflow. |
|
Yes, this seems to work, see my latest commit that builds on top of yours. How do you plan to land this? Will you be making a separate PR with your pretty infrastructure? |
|
Opened #163721 with dataflow pretty printing. |
|
By the way, I'm not super happy about the name |
This comment has been minimized.
This comment has been minimized.
b1a44c7 to
3f7db5c
Compare
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_ir |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| // This pass computes "kill points" for each local, indicating the location of | ||
| // their last use in a particular control flow branch. These are later used in | ||
| // the forward pass later to end the live range of locals that are never | ||
| // borrowed at their last direct use. |
There was a problem hiding this comment.
The support for kill points introduces substantial complexity into the analysis. Could it perhaps be done as a separate optimization? For example by extending dead store elimination to turn copies into moves in statements, similarly to what is already done for calls.
There was a problem hiding this comment.
That doesn't fully cover all the things that the kill point analysis does. In particular:
- If the last use of a local has projections, such as
copy _1.0, changing that tomove _1.0does not kill the local. Only moves of bare locals result in a kill. - If the last use of a local is not an operand, e.g. the index in an index projection, or a call/assignment destination place that is never read.
- If a local is dead never used on one branch, we still need some way of knowing that it should forcibly be killed at the start of that branch for the analysis.
There was a problem hiding this comment.
Yes, the scope of the optimization would be limited.
From my perspective, the motivation is to make review manageable. Starting from a single forward analysis and separate changes to DSE seem trivial in comparison.
There was a problem hiding this comment.
I really think the backwards analysis logic belongs here: a copy->move transformation in DSE would only cover a subset of cases, which means we would need to keep the backwards analysis anyway for precision.
If you prefer I can split the implementation commit into 2 parts, with kill points added later on. These are only used to restrict the lifetime of non-borrowed locals to their last use. Without it, these would follow the same rules as borrowed locals and only end their lifetime on move/StorageDead, so the intermediate analysis is still correct, although conservative.
Also, if any part of this is unclear, do let me know and I will try to improve the comments.
This comment has been minimized.
This comment has been minimized.
5bf6944 to
4d9b471
Compare
| for chunk in kill_points.chunk_by(|a, b| a.1 == b.1) { | ||
| let point = points.point_from_location(chunk[0].1); | ||
| trace!("Kill points at {:?}: {:?}", chunk[0].1, chunk); | ||
| kill_points_map[point] = chunk; |
There was a problem hiding this comment.
nit: perhaps assert that kill_points_map[point] is empty, before the assignment?
| // Notably this kills any dead results produced by a predecessor's | ||
| // terminator. | ||
| state.intersect(&self.kill_points.live_on_entry[block]); |
There was a problem hiding this comment.
Could you also mention that this is no-op for borrowed locals, since live_on_entry is always live for those?
| // StorageLive and StorageDead free the old allocation, even if it has | ||
| // been borrowed. | ||
| if let mir::StatementKind::StorageLive(local) | mir::StatementKind::StorageDead(local) = | ||
| statement.kind | ||
| { | ||
| state.kill(local); | ||
| return; | ||
| } | ||
|
|
||
| // Kill moved operands if the whole local was moved. | ||
| VisitPlacesWith(|place: Place<'tcx>, ctxt| { | ||
| if ctxt == PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) { | ||
| if let Some(local) = place.as_local() { | ||
| state.kill(local); | ||
| } | ||
| } | ||
| }) | ||
| .visit_statement(statement, location); | ||
|
|
||
| // Gen destination places. | ||
| VisitPlacesWith(|place: Place<'tcx>, ctxt| match DefUse::for_place(place, ctxt) { | ||
| DefUse::Def | DefUse::PartialWrite => state.gen_(place.local), | ||
| DefUse::Use | DefUse::NonUse => {} | ||
| }) | ||
| .visit_statement(statement, location); | ||
|
|
||
| // Apply kill points at this statement: if a variable is dead then it | ||
| // doesn't need storage. | ||
| let point = self.points.point_from_location(location); | ||
| for &(local, _) in self.kill_points.kill_points_map[point] { | ||
| state.kill(local); | ||
| } |
There was a problem hiding this comment.
Hmm, I feel like we are missing an abstraction that would describe semantics of statements and terminators in terms of some basic operations: storage live, storage dead, allocate, deallocate, read, and write (implemented in a forward variant and a backward variant). At the moment each step has to implement this separately.
| } | ||
|
|
||
| // End the lifetimes of all locals at the end of the block. Successor | ||
| // blocks (which may not be continuous in the index space!) will |
There was a problem hiding this comment.
nit: s/continuous/contiguous/
| @@ -0,0 +1,241 @@ | |||
| //@ needs-unwind | |||
There was a problem hiding this comment.
Could you also add some tests with indexing and dreferences?
Depends on #163335
This PR implements the lifetime analysis used by the
MoveEliminationpass from rust-lang/rfcs#3943.PreciseLivenesscalculates, at a sub-statement granularity, the points in a function where a local requires storage to be allocated. This is more fine-grained thanMaybeStorageLive, and takes borrows into account.r? tmiasko