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

Split rustc_mir #80522

Merged
merged 7 commits into from
Sep 8, 2021
Merged

Split rustc_mir #80522

merged 7 commits into from
Sep 8, 2021

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Dec 30, 2020

The rustc_mir crate is the second largest in the compiler.
This PR splits it up into 5 crates:

  • rustc_borrowck;
  • rustc_const_eval;
  • rustc_mir_dataflow;
  • rustc_mir_transform;
  • rustc_monomorphize.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020
@cjgillot cjgillot force-pushed the borrowcrate branch 2 times, most recently from 85ccd81 to f53a7e0 Compare December 30, 2020 18:34
@petrochenkov
Copy link
Contributor

The old dependency graph is

// nobody else depends on rustc_mir
rustc_mir -> rustc_interface

, the new dependency graph is

// nobody else depends on rustc_mir
rustc_mir -> rustc_borrowck -> rustc_interface

Does this actually improve build performance?

@petrochenkov
Copy link
Contributor

r? @ecstatic-morse on the specifics (as gitHub suggests) in case this improves the build in general (in which I'm not sure).

@andjo403
Copy link
Contributor

andjo403 commented Jan 1, 2021

some strange results from cargo timing for this PR on my 32 thread cpu.
It takes more time to compile rustc_mir with this PR.
do not know if it is due to all cpus is already busy at the time when rustc_mir is compiled.
this is the results from the step "Building stage0 compiler artifacts" when using the command CARGOFLAGS_BOOTSTRAP=-Ztimings ./x.py build --stage 1
cargo timing for this pr
cargo timing before this pr

@cjgillot
Copy link
Contributor Author

cjgillot commented Jan 2, 2021

This new version now splits the rustc_mir crate into four:

  • rustc_monomorphize ;
  • rustc_mir_transform : holds MIR opts ;
  • rustc_borrowck : holds the borrow checker ;
  • rustc_mir : holds the dataflow framework, and const-eval.

@andjo403
Copy link
Contributor

andjo403 commented Jan 2, 2021

@bors
Copy link
Contributor

bors commented Jan 3, 2021

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

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jan 4, 2021

@cjgillot Please update the PR summary and title to reflect the status quo.

I think this is the right direction. Even though rustc_mir is not on the critical path for a fresh compile, it will improve compile times for people working on MIR transforms. Splitting up crates makes for better APIs in the long run as well, since people are forced to think about encapsulation.

The dataflow framework should also be separable with little effort. It depends on a few util methods for graphviz rendering, which could be moved into the rustc_graphviz crate or into the dataflow crate itself, and the Borrow indexes for defining the Borrows dataflow analysis, which can be moved into rustc_borrowck. After this, rustc_mir could become rustc_const_eval and util could be split up between its consumers and rustc_middle.

I think it's beneficial to do these big refactorings all in one go, so that people don't have to adjust their workflow for the intermediate state, only for it to change again a few months later. I also think you should try to publicize these changes somehow.

r? @wesleywiser (compiler-team lead and MIR opt person) for a final sign-off or referral to MCP.

@bors

This comment has been minimized.

@cjgillot cjgillot changed the title Split the borrow checker to its own crate Split rustc_mir Jan 5, 2021
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@@ -296,7 +296,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// Note that for a given layout, this operation will either always fail or always
/// succeed! Whether it succeeds depends on whether the layout can be represented
/// in an `Immediate`, not on which data is stored there currently.
pub(crate) fn try_read_immediate(
pub fn try_read_immediate(
Copy link
Member

Choose a reason for hiding this comment

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

Having to expose these methods publicly is not great.... that's the issue with this crate splitting business: it makes it a lot harder to uphold invariants inside modules like the interpreter engine.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (97032a6): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

  • Large improvement in instruction counts (up to -2.0% on full builds of deeply-nested-async)
  • Moderate regression in instruction counts (up to 0.8% on full builds of await-call-tree)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@cjgillot cjgillot deleted the borrowcrate branch September 9, 2021 07:09
@Mark-Simulacrum
Copy link
Member

@cjgillot @oli-obk This seems to have hurt the bootstrap time by perf.r-l.o's measurement and the results for individual benchmarks are also mixed. Is there reason to believe that measurement is flawed? Did we measure before this splitting? (I suppose recompilations with smaller changes may be helped by it...).

@bjorn3
Copy link
Member

bjorn3 commented Sep 14, 2021

perf.r-l.o only checks the bootstra time for individual crates. It doesn't check the bootstrap time when multiple crates can compile at the same time, which is what this was meant to improve.

spastorino pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Oct 6, 2021
rust-lang/rust#80522 split the `rustc_mir` crates into 5 crates, effectively invalidating all the direct links to `rustc_mir` in the docs.

I found this while looking at the Two Phase Borrows doc, which is why I am giving out this PR to fix this.
@Mark-Simulacrum
Copy link
Member

I think the regressions here are unfortunate, but I don't think we're going to do anything about them. I think it's potentially true there's more parallelism here but it's also true that these crates are somewhat interconnected regardless (the dependency tree goes through most of the new crates linearly, rather than as siblings), but regardless, it seems like a better factoring is in theory going to make other aspects of this easier in the future.

Marking as triaged.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 11, 2021
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Oct 13, 2021
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Oct 13, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2022
Remove box syntax from rustc_mir_dataflow and rustc_mir_transform

Continuation of rust-lang#87781, inspired by rust-lang#97239. The usages that this PR removes have not appeared from nothing, instead the usage in `rustc_mir_dataflow` and `rustc_mir_transform` was from rust-lang#80522 which split up `rustc_mir`, and which was filed before I filed rust-lang#87781, so it was using the state from before my PR. But it was merged after my PR was merged, so the `box_syntax` uses were able to survive here. Outside of this introduction due to the code being outside of the master branch at the point of merging of my PR, there was only one other introduction of box syntax, in rust-lang#95159. That box syntax was removed again though in rust-lang#95555. Outside of that, `box_syntax` has not made its reoccurrance in compiler crates.
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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.