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

librustc should not depend on librustc_llvm #41473

Closed
whitequark opened this issue Apr 22, 2017 · 10 comments
Closed

librustc should not depend on librustc_llvm #41473

whitequark opened this issue Apr 22, 2017 · 10 comments
Assignees

Comments

@whitequark
Copy link
Member

I'm adding something to librustc_llvm and changing it rebuilds a lot of rustc libraries. @eddyb said this should not be the case.

@eddyb eddyb self-assigned this Apr 22, 2017
@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

Indeed, only librustc_trans actually needs LLVM, in fact I'd really love to combine the two, since LLVM is only used for the one backend, even if it happens to be our only official one.

@whitequark
Copy link
Member Author

Wouldn't that degrade compile times on rebuild?

@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

Does librustc_llvm actually take a long time to build? Or is linking LLVM super slow?

EDIT For the latter we should use the -sys convention and no higher-level APIs.

@whitequark
Copy link
Member Author

I think it's nontrivial, and statically linking LLVM (I dynamically link, personally) can take a lot of time with the default set of targets (I recall linking it for straight minutes).

@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

@whitequark Yeah we can still separate that out I think, but move anything that's not imports into what is currently librustc_trans - if we can have the crate empty, even better.

@arielb1
Copy link
Contributor

arielb1 commented Apr 22, 2017

Currently, librustc_metadata depends on LLVM for its archive reader, so we'll first have to split that out (unless the archive reader does not require configuring LLVM).

@eddyb
Copy link
Member

eddyb commented Apr 22, 2017

@arielb1 Yeah I think we want to split it so the backend provides file -> metadata blob functionality, and librustc_metadata handles both finding the files and decoding the blob.

@arielb1
Copy link
Contributor

arielb1 commented Apr 22, 2017

@eddyb

Yeah. That would also be the Right Thing for targets with "unconventional linking".

@hanna-kruppe
Copy link
Contributor

This impacts me a lot, as essentially all my rustc work for the next few weeks or more consists of modifying LLVM, occasionally librustc_llvm, and rarely librustc_trans. So I'd volunteer to fix it. I could use some details on what this "file -> metadata blob" API should look like, though. Please elaborate here or ping me on IRC.

@nagisa
Copy link
Member

nagisa commented Apr 23, 2017

@rkruppe maybe something like this?

// in metadata
trait MetadataStore {
    fn get_metadata(&self) -> Result<MetadataBlob, String>;
    ...
}

// in trans or somewhere else
impl MetadataStore for LlvmMetadataStore { ... }

// in driver
session.metadata_store_impl: Box<MetadataStore> = Box::new(LlvmMetadataStore);  // or something like that

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 15, 2017
Make only rustc_trans depend on rustc_llvm

With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable
Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans).

What I *didn't* implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened rust-lang#41699 for that step.

Fixes rust-lang#41473
frewsxcv added a commit to frewsxcv/rust that referenced this issue May 15, 2017
Make only rustc_trans depend on rustc_llvm

With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable
Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans).

What I *didn't* implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened rust-lang#41699 for that step.

Fixes rust-lang#41473
bors added a commit that referenced this issue May 16, 2017
Make only rustc_trans depend on rustc_llvm

With these changes, only rustc_trans depends directly on rustc_llvm (and no crate gained a new dependency on trans). This means changing LLVM doesn't rebuild librustc or rustc_metadata, only rustc_trans, rustc_driver and the rustc executable
Also, rustc_driver technically doesn't know about LLVM any more (of course, it still handles a ton of options that conceptually refer to LLVM, but it delegates their implementation to trans).

What I *didn't* implement was merging most or all of rustc_llvm into rustc_trans. I ran into a nasty bug, which was probably just a silly typo somewhere but I probably won't have the time to figure it out in the next week or two. I opened #41699 for that step.

Fixes #41473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants