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

Refactor borrowck liveness values #117880

Merged
merged 9 commits into from
Nov 26, 2023
Merged

Refactor borrowck liveness values #117880

merged 9 commits into from
Nov 26, 2023

Conversation

lqd
Copy link
Member

@lqd lqd commented Nov 13, 2023

This PR starts cleaning up rustc_borrowck, in particular around liveness values:

  • refactors simple names that make no sense anymore: either referring to older structures using region elements, or to bitset containers and values.
  • improves comments and fixes others
  • removes unused return values and unneeded generic arguments

r? @matthewjasper

@rustbot rustbot added 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. labels Nov 13, 2023
@lqd lqd force-pushed the liveness-values branch 2 times, most recently from 3abf0e3 to 1bd570e Compare November 13, 2023 16:39
@cjgillot
Copy link
Contributor

I'm trying to factor this type out in #115291. This would require undoing part of this PR.

@lqd
Copy link
Member Author

lqd commented Nov 14, 2023

@cjgillot I'm not sure that it would:

Moving RegionValueElements and PointIndex looks good, but the borrowck and dest-prop use LivenessValues in different ways in #115291. The former depends on pretty printing that "shouldn't" be moved out. And I'm also looking into storing the live loans there (because various parts of the borrowck are actually pushing liveness data, and need to be updated to compute the live loans. This causes scope differences between NLLs and polonius and needs to be fixed) and that is not something of interest for dest prop. edit: I've done this in #118175.

The common thing between the two is just a SparseIntervalMatrix<N, PointIndex> with some Location mapping, and as far as I can tell only the contains function in today's LivenessValues is used in dest prop. We could uplift such a point index matrix and use it in both rustc_borrowck and dest prop, it's not clear it'd be a huge improvement but why not. Similar results would probably be achieved by having a dataflow visitor that does the mapping to points, or a function converting MaybeLiveLocals's results to a point index matrix, and so on, where LivenessValues itself wouldn't help.

Summary:

  • I think most of the refactoring parts of Save liveness results for DestinationPropagation #115291 should land, but not the LivenessValues move: rustc_borrowck doesn't need it (as-is it also breaks NLL MIR dumps), and dest prop does not use it more than a point matrix. It also causes suboptimal naming like LivenessValues<Local> with union_regions.
  • I think this PR should land: it doesn't impact your dest prop plans, and neither do the possible changes rustc_borrowck could need from LivenessValues.
  • we could uplift in rustc_mir_dataflow the small data structure shared between borrowck and dest prop.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

LGTM, r=me if you'd like

@lqd
Copy link
Member Author

lqd commented Nov 25, 2023

Thanks Jack! I'd first like to give @cjgillot some time for his thoughts on my last comment.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2023

📌 Commit d203476 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2023
@bors
Copy link
Contributor

bors commented Nov 26, 2023

⌛ Testing commit d203476 with merge 274b524...

@bors
Copy link
Contributor

bors commented Nov 26, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 274b524 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 26, 2023
@bors bors merged commit 274b524 into rust-lang:master Nov 26, 2023
12 checks passed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (274b524): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.3% [4.3%, 4.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.5% [-4.4%, -2.5%] 2
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.527s -> 675.337s (-0.03%)
Artifact size: 313.36 MiB -> 313.38 MiB (0.01%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2023
Centralize live loans maintenance to fix scope differences due to liveness

As found in the recent [polonius crater run](rust-lang#117593 (comment)), NLLs and the location-insensitive polonius computed different scopes on some specific CFG shapes, e.g. the following.

![image](https://github.com/rust-lang/rust/assets/247183/c3649f5e-3058-454e-854e-1a6b336bdd5e)

I had missed that liveness data was pushed from different sources than just the liveness computation: there are a few places that do this -- and some of them may be unneeded or at the very least untested, as no tests changed when I tried removing some of them.

Here, `_6` is e.g. dead on entry to `bb2[0]` during `liveness::trace`, but its regions will be marked as live later during "constraint generation" (which I plan to refactor away and put in the liveness module soon). This should cause the inflowing loans to be marked live, but they were only computed in `liveness::trace`.

Therefore, this PR moves live loan maintenance to `LivenessValues`, so that the various places pushing liveness data will all also update live loans at the same time -- except for promoteds which I don't believe need them, and their liveness handling is already interesting/peculiar.

All the regressions I saw in the initial crater run were related to this kind of shapes, and this change did fix all of them on the [next run](rust-lang#117593 (comment)).

r? `@matthewjasper`

(This will conflict with rust-lang#117880 but whichever lands first is fine by me, the end goal is the same for both)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

7 participants