Skip to content

/rust

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

Allow runtime switching between trans backends #45684

Merged
merged 17 commits into from Jan 21, 2018

Conversation

Reviewers

@alexcrichton alexcrichton

@eddyb eddyb

@nikomatsakis nikomatsakis

@nagisa nagisa

@arielb1 arielb1

Assignees

@eddyb eddyb

Labels
S-waiting-on-author T-compiler
Projects
None yet
Milestone
No milestone
@bjorn3
Copy link
Contributor

bjorn3 commented Nov 1, 2017

The driver callback after_llvm has been removed as it doesnt work with multiple backends.

r? @eddyb

@rust-highfive rust-highfive assigned eddyb Nov 1, 2017

@bjorn3 bjorn3 referenced this pull request Nov 1, 2017

Closed

[driver breaking] Rename rustc_trans to rustc_codegen_llvm and allow runtime switching between trans backends #45677

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 1, 2017 •
edited

cc @alexcrichton @rust-lang/compiler I'm not sure a flag is the right thing to use here.
Long-term, do we expect/want to have -C backend=cranelift/llvm/etc. or different drivers?
@bjorn3

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bjorn3 commented Nov 1, 2017

Most of the driver is the same for each backend. We could pass different compiler callbacks to rustc_driver for each backend.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Nov 1, 2017

This seems to add -Z trans but never uses it? How can this be implemented unless we use trait objects?
@bjorn3

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bjorn3 commented Nov 1, 2017

-Z trans is used at https://github.com/bjorn3/rust/blob/67f54e64464ae444de565cc48dc83c7abe6fd55e/src/librustc_driver/lib.rs#L244

@kennytm kennytm added S-waiting-on-review T-compiler labels Nov 1, 2017

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 1, 2017

@bjorn3 We use "custom drivers" to refer to different binaries using rustc_driver, not replacements for rustc_driver.
@bjorn3

bjorn3 reviewed Nov 2, 2017
View changes

src/librustc_trans_utils/trans_crate.rs Outdated
type OngoingCrateTranslation;
type TranslatedCrate;

This comment has been minimized.

Sign in to view
Copy link
@bjorn3

bjorn3 Nov 2, 2017

Author Contributor

Now with less object unsafe associated types 🎊
@carols10cents

This comment has been minimized.

Sign in to view
Copy link
Member

carols10cents commented Nov 6, 2017

What's the status of this PR @bjorn3 @alexcrichton @eddyb? I'm having a tough time telling :)
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 6, 2017

I'm not sure what we should do here. Nominating for discussion in the next compiler meeting.

@eddyb eddyb added the I-nominated label Nov 6, 2017

@bjorn3

bjorn3 reviewed Nov 7, 2017
View changes

src/librustc_trans/lib.rs Outdated
@@ -157,10 +157,6 @@ impl LlvmTransCrate {
}

impl rustc_trans_utils::trans_crate::TransCrate for LlvmTransCrate {
type MetadataLoader = metadata::LlvmMetadataLoader;
type OngoingCrateTranslation = back::write::OngoingCrateTranslation;
type TranslatedCrate = CrateTranslation;

This comment has been minimized.

Sign in to view
Copy link
@bjorn3

bjorn3 Nov 7, 2017

Author Contributor

Now with less object unsafe associated types 🎊

Edit: now completely object safe
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 9, 2017

@bjorn3 Out of curiosity, do you have some specific motivation/need behind this PR, or is it intended as a general refactoring towards supporting multiple backends?
@bjorn3

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bjorn3 commented Nov 10, 2017 •
edited by eddyb

Mainly refactoring, but when this is merged I want to try to add basic cranelift support.
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 10, 2017

I'd suggest more coordination between you, @sunfishcode and me, and the compiler team in general. Just to avoid stepping on eachother's toes, as we are all interested in this goal :).

@bjorn3 bjorn3 force-pushed the bjorn3:runtime_choose_trans2 branch from eae208e to fd38507 Nov 11, 2017

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Nov 12, 2017

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 12, 2017

@bjorn3

bjorn3 reviewed Nov 12, 2017
View changes

src/test/run-make/llvm-phase/test.rs
@@ -1,87 +0,0 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT

This comment has been minimized.

Sign in to view
Copy link
@bjorn3

bjorn3 Nov 12, 2017

Author Contributor

Removed as it requires the now removed after_llvm callback.
@nikomatsakis

nikomatsakis reviewed Nov 14, 2017
View changes

Copy link
Contributor

nikomatsakis left a comment

So, eddyb kicked this to me, but I actually don't know that I'm the right person to do a detailed review here. I mean I could, but I think that @eddyb and perhaps @michaelwoerister have stronger opinions about how this should be internally architected. @bjorn3 can you and @eddyb sync up at some point and talk it over?
src/librustc_driver/driver.rs Outdated

sess.abort_if_errors();
}
// FIXME: Check if the compilation options are supported for the selected backend.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Nov 14, 2017

Contributor

This seems like an important thing to get right before we land, no? Although I guess it's only a warning, not an error, but I wouldn't want to silently be accepting bad things.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Nov 15, 2017

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

@bjorn3 bjorn3 force-pushed the bjorn3:runtime_choose_trans2 branch from 831a965 to b3a986a Nov 15, 2017

@eddyb

eddyb reviewed Nov 15, 2017
View changes

src/librustc_trans_utils/trans_crate.rs Outdated
@@ -226,7 +226,7 @@ impl TransCrate for MetadataOnlyTransCrate {
};
}
fn provide_extern(&self, providers: &mut Providers) {
self.provide_local(providers)
self.provide(providers)

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Nov 15, 2017

Member

You shouldn't need to do anything here, this is redundant.

This comment has been minimized.

Sign in to view
Copy link
@bjorn3

bjorn3 Nov 15, 2017

Author Contributor

You mean i should remove this line?

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Nov 15, 2017

Member

Yes.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Nov 15, 2017

r? @eddyb -- I'm kicking this back to @eddyb =)

@rust-highfive rust-highfive assigned eddyb and unassigned nikomatsakis Nov 15, 2017

@nikomatsakis nikomatsakis removed the I-nominated label Nov 16, 2017

@bjorn3
Fix ICE
Loading status checks…
a4854e8
@bjorn3

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bjorn3 commented Jan 20, 2018

Fixed ICE
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jan 20, 2018

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

📌 Commit a4854e8 has been approved by eddyb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

⌛️ Testing commit a4854e8 with merge 9aa40b0...

bors added a commit that referenced this pull request Jan 20, 2018

@bors
Auto merge of #45684 - bjorn3:runtime_choose_trans2, r=eddyb 
Allow runtime switching between trans backends

The driver callback after_llvm has been removed as it doesnt work with multiple backends.

r? @eddyb
Loading status checks…
9aa40b0
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

💔 Test failed - status-travis

@bjorn3 bjorn3 force-pushed the bjorn3:runtime_choose_trans2 branch from fb976fb to a30232f Jan 20, 2018

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jan 20, 2018

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

📌 Commit a30232f has been approved by eddyb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

⌛️ Testing commit a30232f with merge 3153d4a...

bors added a commit that referenced this pull request Jan 20, 2018

@bors
Auto merge of #45684 - bjorn3:runtime_choose_trans2, r=eddyb 
Allow runtime switching between trans backends

The driver callback after_llvm has been removed as it doesnt work with multiple backends.

r? @eddyb
Loading status checks…
3153d4a
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 20, 2018

💔 Test failed - status-travis
@shepmaster

This comment has been minimized.

Sign in to view
Copy link
Member

shepmaster commented Jan 21, 2018

The job exceeded the maximum time limit for jobs, and has been terminated.

@bors retry
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 21, 2018

⌛️ Testing commit a30232f with merge 9368a1e...

bors added a commit that referenced this pull request Jan 21, 2018

@bors
Auto merge of #45684 - bjorn3:runtime_choose_trans2, r=eddyb 
Allow runtime switching between trans backends

The driver callback after_llvm has been removed as it doesnt work with multiple backends.

r? @eddyb
Loading status checks…
9368a1e
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jan 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 9368a1e to master...

@bors bors merged commit a30232f into rust-lang:master Jan 21, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

kennytm-githubbot added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 21, 2018

@kennytm-githubbot
📣 Toolstate changed by rust-lang/rust#45684! 
Tested on commit rust-lang/rust@9368a1e.

💔 rls on windows: test-pass → build-fail (cc @nrc).
💔 rls on linux: test-pass → build-fail (cc @nrc).
Loading status checks…
c292576

This was referenced Jan 21, 2018

Merged
Merged
Merged
Closed

@bjorn3 bjorn3 deleted the bjorn3:runtime_choose_trans2 branch Jan 21, 2018

@topecongiro topecongiro referenced this pull request Jan 22, 2018

Merged

Rustup to rustc 1.25.0-nightly (97520ccb1 2018-01-21) #5

@alexcrichton alexcrichton referenced this pull request Jan 23, 2018

Merged

rustc: Load the `rustc_trans` crate at runtime #47671

bors added a commit that referenced this pull request Jan 27, 2018

@bors
Auto merge of #47671 - alexcrichton:trans-c-api-only, r=Mark-Simulacrum 
rustc: Load the `rustc_trans` crate at runtime

Building on the work of #45684 this commit updates the compiler to
unconditionally load the `rustc_trans` crate at runtime instead of linking to it
at compile time. The end goal of this work is to implement #46819 where rustc
will have multiple backends available to it to load.

This commit starts off by removing the `extern crate rustc_trans` from the
driver. This involved moving some miscellaneous functionality into the
`TransCrate` trait and also required an implementation of how to locate and load
the trans backend. This ended up being a little tricky because the sysroot isn't
always the right location (for example `--sysroot` arguments) so some extra code
was added as well to probe a directory relative to the current dll (the
rustc_driver dll).

Rustbuild has been updated accordingly as well to have a separate compilation
invocation for the `rustc_trans` crate and assembly it accordingly into the
sysroot. Finally, the distribution logic for the `rustc` package was also
updated to slurp up the trans backends folder.

A number of assorted fallout changes were included here as well to ensure tests
pass and such, and they should all be commented inline.
Loading status checks…
258ebe6

bors added a commit that referenced this pull request Jan 27, 2018

@bors
Auto merge of #47671 - alexcrichton:trans-c-api-only, r=Mark-Simulacrum 
rustc: Load the `rustc_trans` crate at runtime

Building on the work of #45684 this commit updates the compiler to
unconditionally load the `rustc_trans` crate at runtime instead of linking to it
at compile time. The end goal of this work is to implement #46819 where rustc
will have multiple backends available to it to load.

This commit starts off by removing the `extern crate rustc_trans` from the
driver. This involved moving some miscellaneous functionality into the
`TransCrate` trait and also required an implementation of how to locate and load
the trans backend. This ended up being a little tricky because the sysroot isn't
always the right location (for example `--sysroot` arguments) so some extra code
was added as well to probe a directory relative to the current dll (the
rustc_driver dll).

Rustbuild has been updated accordingly as well to have a separate compilation
invocation for the `rustc_trans` crate and assembly it accordingly into the
sysroot. Finally, the distribution logic for the `rustc` package was also
updated to slurp up the trans backends folder.

A number of assorted fallout changes were included here as well to ensure tests
pass and such, and they should all be commented inline.
Loading status checks…
18342df

bors added a commit that referenced this pull request Jan 28, 2018

@bors
Auto merge of #47671 - alexcrichton:trans-c-api-only, r=Mark-Simulacrum 
rustc: Load the `rustc_trans` crate at runtime

Building on the work of #45684 this commit updates the compiler to
unconditionally load the `rustc_trans` crate at runtime instead of linking to it
at compile time. The end goal of this work is to implement #46819 where rustc
will have multiple backends available to it to load.

This commit starts off by removing the `extern crate rustc_trans` from the
driver. This involved moving some miscellaneous functionality into the
`TransCrate` trait and also required an implementation of how to locate and load
the trans backend. This ended up being a little tricky because the sysroot isn't
always the right location (for example `--sysroot` arguments) so some extra code
was added as well to probe a directory relative to the current dll (the
rustc_driver dll).

Rustbuild has been updated accordingly as well to have a separate compilation
invocation for the `rustc_trans` crate and assembly it accordingly into the
sysroot. Finally, the distribution logic for the `rustc` package was also
updated to slurp up the trans backends folder.

A number of assorted fallout changes were included here as well to ensure tests
pass and such, and they should all be commented inline.
Loading status checks…
87990a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.