rewrite liveness analysis to be based on MIR

The current liveness code does a simple liveness computation (actually a few such things) and tells you when e.g. assignments are dead and that sort of thing. It does this on the HIR. It would be better to do this on the MIR — in fact, the NLL computation is already computing liveness across all of MIR, so we ought to be able to piggy back on those results I imagine.

It may be a good idea to wait though until the MIR borrowck stuff "settles down" a bit before attempting this.

I'm removing WG-compiler-nll as this isn't really related to NLL, it's just general cleanup. Here are some tips into the code for future reference:

The liveness code I am referring to, which ought to be ported:

rust/src/librustc/middle/liveness.rs

Line 190 in 860d169

pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {

The MIR-based liveness computation:

rust/src/librustc_mir/util/liveness.rs

Line 118 in 860d169

    
           pub fn liveness_of_locals<'tcx>(mir: &Mir<'tcx>, mode: LivenessMode) -> LivenessResult {

Is this issue still relevant? Can I pick it up?

It's still relevant. Feel free to ping me with questions.

@pnkfelix

I looked into this this morning. A few roadblocks remain.

For one, the MIR has no record of statements like let _ = x even before SimplifyCfg. More complex expressions seem to be lowered even when they are assigned to _, so I think the solution is to look for an assignment of an ExprKind::VarRef specifically and lower it to a FakeRead of some kind.

There's also the issue of associating Locals with their HirIds for diagnostics. This was attempted in #51275, but it caused issues with incremental compilation. That PR worked around the issue by computing the spans needed for MIR borrowck errors ahead-of-time. I think a better solution would be to stop including VarBindingInfo in the StableHash of a MIR body. I don't fully understand the implications of ignoring VarBindingInfo for the purposes of incremental. I think it's okay as long as we don't use the HirId to determine whether to emit a lint/error, only to associate it with a Span. This suggests that we need to precompute the level for the "unused variables" lint. Alternatively, we could duplicate the approach in #51275, but that won't scale as more checking is moved to the MIR.

Finally, I was seeing spurious warnings for variables that were bound in a match arm and only used in a guard. Presumably this will also require some changes to MIR building, but I've not investigated this one yet.

Since I don't have a good intuition for incremental compilation or MIR building, I would appreciate any advice on solving either of these problems. @pnkfelix, this was a while ago, but did you consider ignoring VarBindingInfo during stable hashing in #51275?

The only reason Spans work while HirIds don't is because the incremental tests make stable hashing ignore spans. I think just using HirIds and clearing the VarBindingInfo after borrow checking so that it only affects unoptimized MIR would be fine.

@matthewjasper

I think this agrees with what @matthewjasper said, but I want to encourage us not to do anything "clever" around incremental -- that is to say, we should not try to tweak hashing schemes or something (imo), we should hash all the data. If we don't want something to be hashed, the right way is to remove it from the struct itself (which should then prove we don't have a dependency on it).

That said, when it comes to spans, we've been talking for some time about a better scheme to handle spans and incremental compilation (see e.g. #47389), which seems related.

@nikomatsakis

@nikomatsakis FWIW, my plan was to encapsulate this so that it would be clear to both contributors and reviewers when this HirId was used for something besides diagnostic spans or suggestions.

Regardless, switching VarBindingForm to be a single HirId seems to be somewhat contentious, and doing so is not necessary to fix this issue since we can encode the spans ahead of time like #51275 did. I'll open a separate issue.

Is this something I could contribute to with some mentorship?

I can answer specific questions on anything MIR related, but for the incremental comp stuff we discussed above you'll need to look elsewhere. I never found a solution I was satisfied with.

The other big stumbling block I remember was async fns, since the MIR we generate for them is more disconnected from the HIR than for ordinary fns; some things that appear as used in the HIR appear as unused in the MIR and vice versa. I don't think I tried particularly hard to solve this in #72164, so it's possible you can do better with not much effort.

I was seeing spurious warnings for variables that were bound in a match arm and only used in a guard. Presumably this will also require some changes to MIR building, but I've not investigated this one yet.

the MIR has no record of statements like let _ = x even before SimplifyCfg. More complex expressions seem to be lowered even when they are assigned to _,

These two issues are solved in #72164. See this comment for a discussion of the first.

It's also worth mentioning that with #91032, there's another big HIR dataflow analysis in addition to liveness. Going from 2 big analyses to 1 might be a less attractive proposition than going from 1 to 0.

Auto merge of #142390 - cjgillot:mir-liveness, r=davidtwco

Auto merge of rust-lang#142390 - cjgillot:mir-liveness, r=davidtwco

Auto merge of #142390 - cjgillot:mir-liveness, r=davidtwco

Navigation Menu

Repository navigation

rewrite `liveness` analysis to be based on MIR #51003

21 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

rewrite liveness analysis to be based on MIR #51003

Description

Activity

nikomatsakis commented on Jul 3, 2018

cjgillot commented on Feb 13, 2020

matthewjasper commented on Feb 13, 2020

ecstatic-morse commented on May 10, 2020

matthewjasper commented on May 10, 2020

nikomatsakis commented on May 11, 2020

ecstatic-morse commented on May 11, 2020

camsteffen commented on Jan 24, 2022

ecstatic-morse commented on Jan 25, 2022

21 remaining items

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

rewrite `liveness` analysis to be based on MIR #51003