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 NLL constraint generation and most of polonius fact generation #118216

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

lqd
Copy link
Member

@lqd lqd commented Nov 23, 2023

As discussed in #118175, NLL "constraint generation" is only about liveness, but currently also contains legacy polonius fact generation. The latter is quite messy, and this PR cleans this up to prepare for its future removal:

  • splits polonius fact generation out of NLL constraint generation
  • merges NLL constraint generation to its more natural place, liveness
  • extracts all of the polonius fact generation from NLLs apart from MIR typeck (as fact generation is somewhat in a single place there already, but should be cleaned up) into its own explicit module, with a single entry point instead of many.

There should be no behavior changes, and tests seem to behave the same as master: without polonius, with legacy polonius, with the in-tree polonius.

I've split everything into smaller logical commits for easier review, as it required quite a bit of code to be split and moved around, but it should all be trivial changes.

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 23, 2023
@lqd lqd force-pushed the constraint-generation-non-non branch from 5df5de6 to 5e0f7cd Compare November 23, 2023 22:55
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.

Fairly straightforward refactor. r=me if you'd like

@lqd
Copy link
Member Author

lqd commented Nov 24, 2023

Thanks for taking a look, Jack! It's not particularly urgent as you'd expect, so it's completely fine to wait for a bit in case Matthew doesn't have time to look at this (or they can r=you).

@bors

This comment was marked as resolved.

@lqd lqd force-pushed the constraint-generation-non-non branch from 5e0f7cd to edb3825 Compare November 26, 2023 12:42
@rust-log-analyzer

This comment has been minimized.

@lqd lqd force-pushed the constraint-generation-non-non branch from edb3825 to f969af2 Compare November 26, 2023 13:06
@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 1, 2023

📌 Commit f969af2 has been approved by matthewjasper

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 Dec 1, 2023
@bors
Copy link
Contributor

bors commented Dec 1, 2023

⌛ Testing commit f969af2 with merge caf7300...

@bors
Copy link
Contributor

bors commented Dec 1, 2023

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing caf7300 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2023
@bors bors merged commit caf7300 into rust-lang:master Dec 1, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 1, 2023
@lqd lqd deleted the constraint-generation-non-non branch December 1, 2023 13:58
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (caf7300): 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)
2.6% [2.6%, 2.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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.5%, -2.1%] 3
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 672.931s -> 673.917s (0.15%)
Artifact size: 313.36 MiB -> 313.41 MiB (0.02%)

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