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

Integration of the Cranelift backend with rustc #270

Closed
bjorn3 opened this issue Apr 4, 2020 · 12 comments
Closed

Integration of the Cranelift backend with rustc #270

bjorn3 opened this issue Apr 4, 2020 · 12 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@bjorn3
Copy link
Member

bjorn3 commented Apr 4, 2020

TL;DR

  • Bring my Cranelift based codegen backend in tree using git subtree and gate PR's on it building.
  • Extend the testing infrastructure to annotate tests as being specific for a single backend and
    annotate all LLVM specific tests.

Links and Details

This will first use git subtree to integrate the cg_clif repo. Then changes to bootstrap and
compiletest are needed to be able to build and test it. Next CI will need to gate PR's on cg_clif
building. CI will not run any tests for cg_clif. Finally all add a way to annotate tests as
requiring a specific backend and annotate all LLVM specific tests as such.

Mentors or Reviewers

Maybe @Mark-Simulacrum for bootstrap changes?

@bjorn3 bjorn3 added the major-change A proposal to make a major change to rustc label Apr 4, 2020
@Mark-Simulacrum
Copy link
Member

I think we should wait for clippy subtree to land (just to get the subtree infrastructure, documentation, etc. in place) -- I expect that to happen in the next few weeks. After that, I am willing to help or perhaps implement the build system changes necessary for integrating cranelift's codegen backend.

I suspect we will also want to line up a technical reviewer for compiler integration, though I think I can review that as well (and to some extent I don't know that there will be review required there beyond cleanup that occurs in an ongoing fashion).

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 4, 2020

I think we should wait for clippy subtree to land (just to get the subtree infrastructure, documentation, etc. in place) -- I expect that to happen in the next few weeks.

Agreed

After that, I am willing to help or perhaps implement the build system changes necessary for integrating cranelift's codegen backend.

I think this will involve a partial revert of rust-lang/rust#67077. Before that PR rustc_codegen_llvm was built as dylib after the rest of the compiler the same way that rustc_codegen_cranelift is currently built. That PR also removed the code to load a codegen backend dylib from the sysroot. I don't propose to revert to the state prior, but instead use the code to built rustc_codegen_cranelift as dylib.

@Mark-Simulacrum
Copy link
Member

Hm, so I think that depends. If a build of cranelift is quick enough that we can enable it by default for developers (I hope it is), then I imagine we could statically link both and just have an if in the codegen backend selection.

Even if it isn't quick, I'd strongly prefer that we statically link it (behind a Cargo feature flag or so) just because that is likely to be much easier to get working on various systems, and then expose a -Zcodegen-cranelift flag to switch to it (or perhaps even a #![feature(...)] gate).

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 4, 2020

Statically linking it would require adding all necessary rustc crates to Cargo.toml rather than just getting them from the sysroot. This means that out of tree development of cg_clif would need those dependencies removed from Cargo.toml and added back when updating the subtree.

If a build of cranelift is quick enough that we can enable it by default for developers (I hope it is)

A full rebuild in release mode takes ~2min on my system.

and then expose a -Zcodegen-cranelift flag to switch to it

The way I proposed it, that would be -Zcodegen-backend=cranelift and then rustc would load the dylib from the sysroot.

(or perhaps even a #![feature(...)] gate)

The codegen backend is called immediately after the Session is created to extend the enabled cfg list with the enabled target features. This happens before the first file is parsed.

@nikomatsakis nikomatsakis reopened this Apr 16, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Apr 16, 2020
@spastorino
Copy link
Member

Added to our compiler team meeting agenda, removing to-announce label.

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Apr 23, 2020
@wesleywiser
Copy link
Member

It's been about two months, how has the clippy subtree been working out? Does that still seem like the way to go for cg_clif?

@nikomatsakis
Copy link
Contributor

cc @oli-obk @Manishearth -- I think it's been working out reasonably well, but I'm not sure if we have done the "rust repo -> clippy repo" style merges yet.

@Manishearth
Copy link
Member

@nikomatsakis we've been using git subrepo, not git subtree. But it's been going okay i think. cc @flip1995

@flip1995
Copy link
Member

flip1995 commented Jun 18, 2020

@Manishearth we're using git subtree, but I wouldn't recommend to commit other things to that, since we are only able to use this, since we're using a version from this PR gitgitgadget/git#493, instead of the official git subtree version.

Once you gone through the subtree routine, I would say it is pretty nice, to just do a rustup with a few git commands and maybe a code format, instead of going through PRs to find the culprit.

@flip1995
Copy link
Member

flip1995 commented Jun 18, 2020

More about issues we have with git subtree here: rust-lang/rust-clippy#5565

In-depth documentation on how we use git subtree here: https://github.com/rust-lang/rust-clippy/blob/master/CONTRIBUTING.md#fixing-build-failures-caused-by-rust (note the note at the bottom of the section)

@Manishearth
Copy link
Member

@flip1995 ugh i keep getting confused between subrepo and subtree. thanks

I think it's fine to use a forked subtree especially if fixes are incoming

@spastorino spastorino added the T-compiler Add this label so rfcbot knows to poll the compiler team label Sep 17, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Oct 2, 2020

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Oct 2, 2020
@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Oct 14, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Oct 14, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 15, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 26, 2020
…acrum

Add cg_clif as optional codegen backend

Rustc_codegen_cranelift is an alternative codegen backend for rustc based on Cranelift. It has the potential to improve compilation times in debug mode. In my experience the compile time improvements over debug mode LLVM for a clean build are about 20-30% in most cases.

This PR adds cg_clif as optional codegen backend. By default it is only enabled for `./x.py check`. It can be enabled for `./x.py build` too by adding `cranelift` to the `rust.codegen-backends` array in `config.toml`.

MCP: rust-lang/compiler-team#270

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

9 participants