Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More improvents to dest prop #95700

Closed
wants to merge 3 commits into from
Closed

Conversation

JakobDegen
Copy link
Contributor

The code changes here are not actually that significant, I'm mostly using this PR as a way to document my plans for fixing this opt.

Three commits:

  1. Remove usage of initialization analysis. I believe this was technically sound because MaybeInitializedLocals returns true even if the local is only partially initialized, but it still seems extremely sketchy to me (and likely to break if the analysis ever gets more precise). I also don't think there's a need for it in any case. Uninitialized places should, in general, also be dead. This does break one test, but that test was somewhat flakey already. Essentially what happens there is something like this:

    (_1.0) = _2
    _4 = (_1.0)

    The pass was dest propping _2 into _1.0; however, it did not actually know that _1.0 was dead; instead it knew that it was uninit, and concluded soundness from that. This means that if the place happened to be initialized but dead for whatever unrelated reason, this would not have fired. The way to get this and many other similar examples back is to use a more precise version of liveness analysis.

    This also has the benefit of simplifying the expensive conflict building, and should get us some performance back. This will also make the performance improvements (see below) easier later on.

  2. Previously, we did not check for any of the intra statement conflicts in assignments. The justification for this was that self assignments get removed, but this is only the case for Rvalue::Use, not all the other rvalues. This commit adds in conflict tracking for assignments with all the other types of rvalues. Unfortunately, the handling of the locals used for indexing places is fundamentally sketchy, both here and elsewhere in this pass. It will probably be fairly difficult to land this without a decision for Semantics of MIR assignments, around aliasing, ordering, and primitives. #68364 (for MIR semantics, not for Rust semantics). If/when we make that decision, and if that decision looks like Ralf's suggestion, then I think the real issue here is that there is a missing dataflow state: The one after the place gets retagged and before the RHS is evaluated. Adding in this dataflow state when it exists would completely remove the need for custom behavior around intra-statement conflicts, and seems much more robust in general.

  3. Just documents the outstanding problems

r? @jonas-schievink (feel free to reassign, I have no idea if you have review capacity these days)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2022
@JakobDegen
Copy link
Contributor Author

JakobDegen commented Apr 5, 2022

Oh, I realize I forgot to say what I intended to about perf.

So, there is some "easy" perf lying around here in the following form - Right now, for each statement, we take the whole set of locals and mark all conflicts between any two of them that are both live. This is fundamentally an s^2 step (where s is the number of locals). However, there is no fundamental reason that the analysis could not tell us which locals had their liveness change - we can then record conflicts only for those locals, which is linear instead of quadratic. I'm not going to bother implementing something like this right now, but this is absolutely a thing we should make available once we get smarter liveness analysis.

@jonas-schievink
Copy link
Contributor

I don't really do open source in my free time anymore, so r? @oli-obk

let mut v = RvalueVisitor(self, lhs_local);
let location = Location { block: BasicBlock::MAX, statement_index: 0 };
if let Rvalue::Use(op) = rvalue {
if let Some(rhs_place) = op.place() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave a comment that this explicitly skips the place and only looks for index places in the projection

@@ -507,23 +539,6 @@ impl<'a> Conflicts<'a> {

fn record_terminator_conflicts(&mut self, term: &Terminator<'_>) {
match &term.kind {
TerminatorKind::DropAndReplace {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we need this anymore? Because we run so late this variant doesn't occur? Should probably group the ones that can't happen into a => bug!() arm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropAndReplace gets removed during drop elaboration.

@bors
Copy link
Contributor

bors commented Apr 11, 2022

☔ The latest upstream changes (presumably #95125) made this pull request unmergeable. Please resolve the merge conflicts.

@JakobDegen
Copy link
Contributor Author

I'm going to close this, because after putting in some more thought I'm not sure this is really a productive first step anymore. Something like this will be necessary I feel, but probably in a different form.

@JakobDegen JakobDegen closed this Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants