Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uprustc: Split Emscripten to a separate codegen backend #47730
Conversation
rust-highfive
assigned
pnkfelix
Jan 25, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
rust-highfive
assigned
Mark-Simulacrum
and unassigned
pnkfelix
Jan 25, 2018
alexcrichton
referenced this pull request
Jan 25, 2018
Closed
Implement shipping a per-target LLVM backend #46819
This comment has been minimized.
This comment has been minimized.
|
Lots of tidy error. |
alexcrichton
force-pushed the
alexcrichton:multitrans
branch
from
885a792
to
2ab5374
Jan 25, 2018
kennytm
added
T-compiler
S-waiting-on-review
T-infra
labels
Jan 25, 2018
Mark-Simulacrum
reviewed
Jan 25, 2018
|
So I think I'm mostly happy with this (only reviewing the second commit). Can we check if this will by-default make a clone of rust-lang/rust with |
| [submodule "src/llvm-emscripten"] | ||
| path = src/llvm-emscripten | ||
| url = https://github.com/rust-lang/llvm | ||
| branch = rust-llvm-release-4-0-1 |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jan 25, 2018
Member
Shouldn't this be an Emscripten branch? It seems odd that emscripten has branch = where other submodules don't.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 25, 2018
Author
Member
Indeed! I figured that for now it's probably easiest to do the smallest delta possible which is to switch to the same version we have right now. I think if we upgrade in the future though we should try to depend directly on Emscripten's fork of LLVM.
As for the branch = I think that's because I passed a -b argument to git submodule add, but as far as I can tell this is never used when cloning (as the submodule has a revision). It may be used for shallow clones of submodules but I never got that working historically..
| const DEFAULT: bool = true; | ||
|
|
||
| fn should_run(run: ShouldRun) -> ShouldRun { | ||
| run.path("src/librustc_trans").krate("rustc_trans") |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jan 25, 2018
Member
krate here is useless in theory since that crate doesn't have a crate graph really in src/ AFAIK. Though I guess it doesn't hurt... I'd prefer that we didn't have it though.
| run.builder.ensure(CodegenBackend { | ||
| compiler: run.builder.compiler(run.builder.top_stage, run.host), | ||
| target: run.target, | ||
| backend: INTERNER.intern_str("llvm"), |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jan 25, 2018
Member
This should not be hardcoded here... I would expect this sort of thing in src/bootstrap/config.rs probably.
| } | ||
| if max > stamp_mtime { | ||
| build.verbose(&format!("updating {:?} as {:?} changed", stamp, max_path)); | ||
| } else { | ||
| build.verbose(&format!("updating {:?} as deps changed", stamp)); | ||
| } | ||
| t!(t!(File::create(stamp)).write_all(&new_contents)); | ||
| return deps |
This comment has been minimized.
This comment has been minimized.
| @@ -318,6 +320,7 @@ impl Config { | |||
| config.ignore_git = false; | |||
| config.rust_dist_src = true; | |||
| config.test_miri = false; | |||
| config.rust_codegen_backends = vec![INTERNER.intern_str("llvm")]; | |||
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jan 25, 2018
Member
Yeah, so as I mentioned before I'd expect us to use the first element perhaps from here or something like that in the default backend to compile for in src/bootstrap/compile.rs CodegenBackend.
alexcrichton
force-pushed the
alexcrichton:multitrans
branch
from
2ab5374
to
0ef3ab7
Jan 25, 2018
This comment has been minimized.
This comment has been minimized.
Indeed! I made sure to verify that we don't clone LLVM twice (we don't) and the right thing happens on CI (download from github). Now when you do enable emscripten I think that the two llvm submodules probably won't share objects, but I'm not sure there's much we can do about that... |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Sounds good to me. r=me; I agree that it seems unlikely that we can get git to understand that they're fundamentally the same repository. And that may not be true indefinitely. |
alexcrichton
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Jan 26, 2018
This comment has been minimized.
This comment has been minimized.
alexcrichton
force-pushed the
alexcrichton:multitrans
branch
from
0ef3ab7
to
d061f30
Jan 28, 2018
This comment has been minimized.
This comment has been minimized.
|
@bors: r=Mark-Simulacrum |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jan 28, 2018
This comment has been minimized.
This comment has been minimized.
|
|
kennytm
added
S-waiting-on-author
and removed
S-waiting-on-author
labels
Jan 28, 2018
alexcrichton
force-pushed the
alexcrichton:multitrans
branch
from
d061f30
to
b746667
Jan 28, 2018
This comment has been minimized.
This comment has been minimized.
|
@bors: r=Mark-Simulacrum |
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton commentedJan 25, 2018
This commit introduces a separately compiled backend for Emscripten, avoiding
compiling the
JSBackendtarget in the main LLVM codegen backend. This buildson the foundation provided by #47671 to create a new codegen backend dedicated
solely to Emscripten, removing the
JSBackendof the main codegen backend inthe process.
A new field was added to each target for this commit which specifies the backend
to use for translation, the default being
llvmwhich is the main backend thatwe use. The Emscripten targets specify an
emscriptenbackend instead of themain
llvmone.There's a whole bunch of consequences of this change, but I'll try to enumerate
them here:
will soon start to drift from the Emscripten submodule, but currently they're
both at the same revision.
This is gated behind a
--enable-emscriptenflag to the configure script. Bydefault users should neither check out the emscripten submodule nor compile
it.
init_repo.shscript was updated to fetch the Emscripten submodule fromGitHub the same way we do the main LLVM submodule (a tarball fetch).
of targets on CI. We'll only be shipping an Emscripten backend with Tier 1
platforms, though. All cross-compiled platforms will not be receiving an
Emscripten backend yet.
This commit means that when you download the
rustcpackage in Rustup for Tier1 platforms you'll be receiving two trans backends, one for Emscripten and one
that's the general LLVM backend. If you never compile for Emscripten you'll
never use the Emscripten backend, so we may update this one day to only download
the Emscripten backend when you add the Emscripten target. For now though it's
just an extra 10MB gzip'd.
Closes #46819