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

Remove _all_ codegen dependencies on rustc_mir 🎉 #61062

Merged
merged 12 commits into from
Jun 3, 2019

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented May 23, 2019

This code is pretty self-contained. It has no references to the rest of rustc_mir. Moving it to its own crate means that almost all of the references from rustc_codegen_* to rustc_mir are instead moved to rustc_monomorphize, which should help improve compile times for the compiler a bit...

With the help of eddyb and oli-obk, all of the dependencies of librustc_codegen_* on librustc_mir have been removed:

  • dependencies on rustc_mir::monomorphize were moved to rustc::mir::mono
  • rustc_mir::const_eval::const_field is made into a query.
  • rustc_mir::interpret::type_name is made into a query.

This should help reduce compile time when working on rustc_mir 🕐

cc #47849

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1d07b310:start=1558571992361450311,finish=1558571993165491254,duration=804040943
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:06:59]    Compiling rustc_macros v0.1.0 (/checkout/src/librustc_macros)
[00:07:06]    Compiling syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
[00:07:10]    Compiling rustc_errors v0.0.0 (/checkout/src/librustc_errors)
[00:08:14]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:14:22]    Compiling rustc_monomorphize v0.0.0 (/checkout/src/librustc_monomorphize)
[00:17:25]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:17:26]    Compiling rustc_lint v0.0.0 (/checkout/src/librustc_lint)
[00:18:21]    Compiling rustc_traits v0.0.0 (/checkout/src/librustc_traits)
[00:18:23]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
---
[00:27:27]    Compiling rustc_lsan v0.0.0 (/checkout/src/librustc_lsan)
[00:27:27]    Compiling rustc_msan v0.0.0 (/checkout/src/librustc_msan)
[00:27:28]    Compiling rustc_tsan v0.0.0 (/checkout/src/librustc_tsan)
[00:27:28]    Compiling rustc_asan v0.0.0 (/checkout/src/librustc_asan)
[00:27:53] error: internal compiler error: src/librustc/ty/query/mod.rs:101: tcx.collect_and_partition_mono_items(crate0) unsupported by its crate
[00:27:53] thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:637:9
[00:27:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:27:53] error: aborting due to previous error
[00:27:53] 
[00:27:53] 
[00:27:53] 
[00:27:53] note: the compiler unexpectedly panicked. this is a bug.
[00:27:53] 
[00:27:53] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:27:53] 
[00:27:53] note: rustc 1.36.0-dev running on x86_64-unknown-linux-gnu
[00:27:53] 
[00:27:53] note: compiler flags: -Z external-macro-backtrace -Z unstable-options -Z force-unstable-if-unmarked -C opt-level=2 -C prefer-dynamic -C debug-assertions=y -C codegen-units=1 -C link-args=-Wl,-rpath,$ORIGIN/../lib --crate-type lib
[00:27:53] note: some of the compiler flags provided by cargo are hidden
[00:27:53] 
[00:27:53] error: Could not compile `core`.
[00:27:53] 
---
19656 ./obj/build/x86_64-unknown-linux-gnu/stage0-std/release
19256 ./src/llvm-project/lldb/www/cpp_reference
19252 ./src/llvm-project/lldb/www/cpp_reference/html
travis_time:end:1664d2d0:start=1558573677533508712,finish=1558573678169073130,dura[0K$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:00d1a2ee
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@mark-i-m
Copy link
Member Author

All tests passing locally now :)

}
}

pub fn custom_coerce_unsize_info<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

cc @oli-obk Surely this could be somewhere that miri would also benefit from?

Copy link
Contributor

Choose a reason for hiding this comment

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

since librustc_monomorphize is a dependency of librustc_mir, that should be easy enough

@eddyb
Copy link
Member

eddyb commented May 23, 2019

Looks like the non-reexport items are mostly extension traits. We should figure out a way to break the dependency without necessarily moving the code out of rustc_mir.

@mark-i-m
Copy link
Member Author

@eddyb I think I addressed most of your review concerns.

Looks like the non-reexport items are mostly extension traits. We should figure out a way to break the dependency without necessarily moving the code out of rustc_mir.

I thought about this a bit. It might also be possible to move them to librustc directly, if that seems appropriate. That is, I don't think there are any cyclic dependencies.

@mark-i-m
Copy link
Member Author

In fact, that may also enable moving parts of librustc_codegen_utils to librustc too, but I haven't checked.

@eddyb
Copy link
Member

eddyb commented May 23, 2019

@mark-i-m I quickly looked at it, and both MonoItemExt and CodegenUnitExt should be movable to rustc::mir::mono as inherent methods.

@mark-i-m
Copy link
Member Author

@eddyb yes, but that trait uses this one.

@eddyb
Copy link
Member

eddyb commented May 24, 2019

@mark-i-m Yes, but not in a way which needs rustc_codegen_ssa::mono_item::MonoItemExt to be moved, in order to move rustc_mir::monomorphize::item::MonoItemExt to inherent methods.

@mark-i-m
Copy link
Member Author

@eddyb Done. librustc_codegen_llvm now no longer depends on librustc_mir or librustc_monomorphize 🎉

src/librustc_mir/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Since all the changes seemed to have been a success, can we keep monomorphize in rustc_mir now, and still get the benefits of codegen not depending on rustc_mir 😁?

@mark-i-m
Copy link
Member Author

@eddyb Yes, I will move monomorphize back to rustc_mir.

However, there is still one there is still one dependency from librustc_codegen_ssa to librustc_mir: rustc_mir::const_eval::const_field. However, it looks like that might also be movable to librustc (not sure though).

@mark-i-m
Copy link
Member Author

const_field is harder to move because it uses a lot of the const eval machinery in rustc_mir::interpret. @oli-obk any suggestions?

@eddyb
Copy link
Member

eddyb commented May 24, 2019

r? @oli-obk for rustc_mir::const_eval::const_field

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb May 24, 2019
@mark-i-m mark-i-m changed the title Turn rustc_mir::monomorphize into librustc_monomorphize Remove most codegen dependencies on rustc_mir May 24, 2019
@mark-i-m
Copy link
Member Author

mark-i-m commented Jun 3, 2019

@eddyb Rebased and addressed nit. Unfortunately, someone added another dependency on rustc_mir in the meantime (on rustc_mir::interpret::type_name), so I turned that into a query too in the last commit. Let me know what you think.

@mark-i-m
Copy link
Member Author

mark-i-m commented Jun 3, 2019

Also, I'm glad to address nits, but if we could do it in a followup PR, that would be great, as this rebase was painful...

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2019

someone added another dependency on rustc_mir in the meantime (on rustc_mir::interpret::type_name), so I turned that into a query too in the last commit. Let me know what you think.

Cc @oli-obk #60166

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

@bors r=eddyb,oli-obk p=1

type_name lgtm, prioritizing due to bitrottiness

@bors
Copy link
Contributor

bors commented Jun 3, 2019

📌 Commit 0f822d7 has been approved by eddyb,oli-obk

@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 Jun 3, 2019
@bors
Copy link
Contributor

bors commented Jun 3, 2019

⌛ Testing commit 0f822d7 with merge da9ebc8...

bors added a commit that referenced this pull request Jun 3, 2019
Remove _all_ codegen dependencies on `rustc_mir` 🎉

~This code is pretty self-contained. It has no references to the rest of `rustc_mir`. Moving it to its own crate means that almost all of the references from `rustc_codegen_*` to `rustc_mir` are instead moved to `rustc_monomorphize`, which should help improve compile times for the compiler a bit...~

With the help of eddyb and oli-obk, all of the dependencies of `librustc_codegen_*` on `librustc_mir` have been removed:
- dependencies on `rustc_mir::monomorphize` were moved to `rustc::mir::mono`
- `rustc_mir::const_eval::const_field` is made into a query.
- `rustc_mir::interpret::type_name` is made into a query.

This should help reduce compile time when working on `rustc_mir` 🕐

cc #47849

r? @eddyb
@bors
Copy link
Contributor

bors commented Jun 3, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb,oli-obk
Pushing da9ebc8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2019
@bors bors merged commit 0f822d7 into rust-lang:master Jun 3, 2019
@eddyb
Copy link
Member

eddyb commented Jun 3, 2019

@oli-obk We should probably put miri intrinsics behind const_eval with the DefId of the intrinsic (effectively treating a 0-argument call as a use of a constant).

@oli-obk
Copy link
Contributor

oli-obk commented Jun 3, 2019

so... const fns with zero arguments and constants become indistinguishable?

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants