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.

Sign up for GitHub

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

Jump to bottom

Move rustllvm into compiler/rustc_llvm #74787

Merged
merged 1 commit into from
Sep 10, 2020
Merged

Move rustllvm into compiler/rustc_llvm #74787

merged 1 commit into from
Sep 10, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 26, 2020 •
edited
Loading

The rustllvm directory is not self-contained, it contains C++ code built by a build script of the rustc_llvm crate which is then linked into that crate.
So it makes sense to make rustllvm a part of rustc_llvm and move it into its directory.
I replaced rustllvm with more obvious llvm-wrapper as the subdirectory name, but something like llvm-adapter would work as well, other suggestions are welcome.

To make things more confusing, the Rust side of FFI functions defined in rustllvm can be found in rustc_codegen_llvm rather than in rustc_llvm. Perhaps they need to be moved as well, but this PR doesn't do that.

The presence of multiple LLVM-related directories in src (llvm-project, rustllvm, librustc_llvm, librustc_codegen_llvm and their predecessors) historically confused me and made me wonder about their purpose.
With this PR we will have LLVM itself (llvm-project), a FFI crate (rustc_llvm, kind of llvm-sys) and a codegen backend crate using LLVM through the FFI crate (rustc_codegen_llvm).

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Jul 26, 2020
@petrochenkov
Copy link
Contributor Author

cc interested parties @cuviper @nikic @bjorn3 @eddyb @mark-i-m (I could forget someone else)

@lcnr
Copy link
Contributor

lcnr commented Jul 26, 2020

🤷 There aren't any obvious problems here but it's not up to me to decide if this change should be made.

r? @cuviper

@rust-highfive rust-highfive assigned cuviper and unassigned lcnr Jul 26, 2020
@cuviper
Copy link
Member

cuviper commented Jul 27, 2020

The rustllvm directory is not self-contained, it contains C++ code built by a build script of the rustc_llvm crate which is then linked into that crate.
So it makes sense to make rustllvm a part of rustc_llvm and move it into its directory.
I replaced rustllvm with more obvious llvm-wrapper as the subdirectory name, but something like llvm-adapter would work as well, other suggestions are welcome.

This all sounds good to me.

To make things more confusing, the Rust side of FFI functions defined in rustllvm can be found in librustc_codegen_llvm rather than in rustc_llvm. Perhaps they need to be moved as well, but this PR doesn't do that.

I think we could even move all of the FFI declarations to rustc_llvm, not just the LLVMRust* stuff -- as you said, this crate already acts like an llvm-sys kind of crate. Maybe even the whole rustc_codegen_llvm::llvm module should move.

If this is merged, then the merge should probably be done after the compiler part of rust-lang/compiler-team#316 to avoid moving things around twice.

I'm personally not too worried about that, as this seems like a simple enough move that git's tracking should work, but it's ok if you want to wait and do it with the compiler move or soon after.

We should also remember to update the config in highfive.

@petrochenkov petrochenkov mentioned this pull request Jul 28, 2020
Merged
@petrochenkov
Copy link
Contributor Author

Blocked on #74862.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2020
@bors
Copy link
Contributor

bors commented Jul 29, 2020

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

@matklad
Copy link
Member

matklad commented Aug 30, 2020

a FFI crate (librustc_llvm, kind of llvm-sys

Shall we perhaps just rename it to llvm-sys?

@mark-i-m
Copy link
Member

Yes, I agree the naming between librustc_llvm, rustllvm, and llvm-project are not particularly clear...

@mark-i-m
Copy link
Member

Also, this is no longer blocked, I think.

@rustbot modify labels: +S-waiting-on-author -S-blocked

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Aug 30, 2020
@petrochenkov petrochenkov changed the title [WIP] Move rustllvm into librustc_llvm Move rustllvm into compiler/rustc_llvm Sep 2, 2020
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 2, 2020
@petrochenkov
Copy link
Contributor Author

Rebased.
src/librustc_llvm is also moved to compiler/rustc_llvm consistently with #74862.

It's not renamed to llvm-sys to keep naming scheme consistent with all other rustc crates.

rustc_codegen_llvm::llvm is not yet moved to rustc_llvm because it's somewhat intertwined with the rest of rustc_codegen_llvm, so untangling them would better be done in a separate PR.

@petrochenkov
Copy link
Contributor Author

It's not renamed to llvm-sys to keep naming scheme consistent with all other rustc crates.

It could be renamed from rustc_llvm to rustc_llvm_ffi for clarity though.

@bors
Copy link
Contributor

bors commented Sep 4, 2020

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

cuviper
cuviper approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Need to pick up the #75655 change, then r=me.

@petrochenkov
Copy link
Contributor Author

@bors r=cuviper

@bors
Copy link
Contributor

bors commented Sep 9, 2020

📌 Commit 10d3f8a has been approved by cuviper

@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 Sep 9, 2020
@bors
Copy link
Contributor

bors commented Sep 10, 2020

⌛ Testing commit 10d3f8a with merge 6744b6415ee1b69b4a5ea618632557446fba6004...

tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2020
@tmandry
Rollup merge of rust-lang#74787 - petrochenkov:rustllvm, r=cuviper
7074e02 
Move `rustllvm` into `compiler/rustc_llvm`

The `rustllvm` directory is not self-contained, it contains C++ code built by a build script of the `rustc_llvm` crate which is then linked into that crate.
So it makes sense to make `rustllvm` a part of `rustc_llvm` and move it into its directory.
I replaced `rustllvm` with more obvious `llvm-wrapper` as the subdirectory name, but something like `llvm-adapter` would work as well, other suggestions are welcome.

To make things more confusing, the Rust side of FFI functions defined in `rustllvm` can be found in `rustc_codegen_llvm` rather than in `rustc_llvm`. Perhaps they need to be moved as well, but this PR doesn't do that.

The presence of multiple LLVM-related directories in `src` (`llvm-project`, `rustllvm`, `librustc_llvm`, `librustc_codegen_llvm` and their predecessors) historically confused me and made me wonder about their purpose.
With this PR we will have LLVM itself (`llvm-project`), a FFI crate (`rustc_llvm`, kind of `llvm-sys`) and a codegen backend crate using LLVM through the FFI crate (`rustc_codegen_llvm`).
@tmandry tmandry mentioned this pull request Sep 10, 2020
Closed
@tmandry
Copy link
Member

tmandry commented Sep 10, 2020

Rolled up
@bors retry

@bors
Copy link
Contributor

bors commented Sep 10, 2020

⌛ Testing commit 10d3f8a with merge 92a4fcb95c9c085e579bfeb938a1d490b115483d...

@tmandry tmandry mentioned this pull request Sep 10, 2020
Merged
@tmandry
Copy link
Member

tmandry commented Sep 10, 2020

Rolled up again
@bors retry

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 10, 2020
@bors
Auto merge of rust-lang#76558 - tmandry:rollup-bskim2r, r=tmandry
a1894e4 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#74787 (Move `rustllvm` into `compiler/rustc_llvm`)
 - rust-lang#76458 (Add drain_filter method to HashMap and HashSet)
 - rust-lang#76472 (rustbuild: don't set PYTHON_EXECUTABLE and WITH_POLLY cmake vars since they are no longer supported by llvm)
 - rust-lang#76497 (Use intra-doc links in `core::ptr`)
 - rust-lang#76500 (Add -Zgraphviz_dark_mode and monospace font fix)
 - rust-lang#76543 (Document btree's unwrap_unchecked)
 - rust-lang#76556 (Revert rust-lang#76285)

Failed merges:

r? `@ghost`
@bors
Copy link
Contributor

bors commented Sep 10, 2020

⌛ Testing commit 10d3f8a with merge a1894e4...

@bors bors merged commit f09372a into rust-lang:master Sep 10, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 10, 2020
cuviper added a commit to cuviper/highfive that referenced this pull request Sep 10, 2020
@cuviper
Update rust.json for compiler/rustc_llvm
f48bce3 
`src/rustllvm` moved to `compiler/rustc_llvm` in rust-lang/rust#74787
cuviper added a commit to cuviper/highfive that referenced this pull request Sep 10, 2020
@cuviper
Update rust.json for compiler/rustc_llvm
f6d5453 
`src/rustllvm` and `src/librustc_llvm` moved to `compiler/rustc_llvm` in
rust-lang/rust#74787.
cuviper added a commit to cuviper/highfive that referenced this pull request Sep 10, 2020
@cuviper
Update rust.json for compiler/rustc_llvm
b88da5c 
`src/rustllvm` and `src/librustc_llvm` moved to `compiler/rustc_llvm` in
rust-lang/rust#74787.
@cuviper cuviper mentioned this pull request Sep 10, 2020
Merged
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Jan 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper approved these changes

Assignees

@cuviper cuviper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
9 participants
@petrochenkov @rust-highfive @lcnr @cuviper @bors @matklad @mark-i-m @tmandry @rustbot