Stream: t-compiler/major changes

Topic: Integration of the Cranelift backend with… compiler-team#270


triagebot (Oct 02 2020 at 10:09, on Zulip):

@T-compiler: Proposal #270 has been seconded, and will be approved in 10 days if no objections are raised.

oli (Oct 02 2020 at 10:10, on Zulip):

previous discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/design%20meeting%202020-04-03%20compiler-team%23257/near/192806450

oli (Oct 02 2020 at 10:11, on Zulip):

It's working well for clippy, and as long as @bjorn3 is the one syncing (and using the patched git subtree), there's no additional work on our side beyond keeping the backend building

bjorn3 (Oct 02 2020 at 10:27, on Zulip):

I got the patched git-subtree installed locally using make prefix=$HOME/.local/; make prefix=$HOME/.local/ install; cd contrib/subtree; make prefix=$HOME/.local/; make prefix=$HOME/.local/ install.

bjorn3 (Oct 02 2020 at 10:28, on Zulip):

I already have had a rust branch for the integration for a while now. It has a few things I want to change, but those should be easy.

oli (Oct 02 2020 at 11:46, on Zulip):

sorry for forgetting about this MCP for so long. I only saw it again yesterday in the compiler team meeting

bjorn3 (Oct 02 2020 at 11:47, on Zulip):

No problem. I was actually waiting for the git subtree PR to be merged, but that has been taking long enough.

bjorn3 (Oct 02 2020 at 11:48, on Zulip):

Should I submit a PR once it is ready, or wait for the fcp?

oli (Oct 02 2020 at 12:08, on Zulip):

let's wait for after the stable release next week, so we can also just wait for the FCP which is just a few days after that

bjorn3 (Oct 02 2020 at 12:09, on Zulip):

ok

RalfJ (Oct 04 2020 at 11:14, on Zulip):

bjorn3 said:

No problem. I was actually waiting for the git subtree PR to be merged, but that has been taking long enough.

still waiting for that for Miri... I am somewhat worried about rust basically depending on a fork of git^^
does anyone have contacts with upstream to figure out what it takes to get the PR through?

bjorn3 (Oct 10 2020 at 14:49, on Zulip):

cg_clif used to use -Zcodegen-backend to integrate with rustc. In #76474 I introduced the alternative of using a custom driver. I ported most of cg_clif to use this interface as it makes it possible to always set certain options for cg_clif and allows adding extra arguments for for example enabling the jit mode instead of having to resort to env vars. As of https://github.com/bjorn3/rustc_codegen_cranelift/commit/faec12461f34db4145000a891c4ee5df2e8d4132 the last remaining thing still requiring -Zcodegen-backend is rustdoc. Unlike rustc, rustdoc doesn't have support for custom drivers. How do I port it too?

Joshua Nelson (Oct 10 2020 at 14:50, on Zulip):

rustdoc uses drivers at all??

bjorn3 (Oct 10 2020 at 14:51, on Zulip):

It doesn't. That is the problem.

bjorn3 (Oct 10 2020 at 14:51, on Zulip):

librustdoc isn't included in the sysroot and only exports main().

Joshua Nelson (Oct 10 2020 at 14:52, on Zulip):

I'm not sure what you're trying to do.

bjorn3 (Oct 10 2020 at 14:53, on Zulip):

I want to override the codegen backend used by rustdoc without passing -Zcodegen-backend. I want an executable that passes an Box<dyn CodegenBackend> and a list of arguments to rustdoc.

bjorn3 (Oct 10 2020 at 14:54, on Zulip):

By the way why does librustdoc actually exist separate from the rustdoc crate, when the rustdoc crate is just fn main() { rustdoc::main() }.

bjorn3 (Oct 10 2020 at 14:55, on Zulip):

For rustc I have https://github.com/bjorn3/rustc_codegen_cranelift/blob/master/src/bin/cg_clif.rs

Joshua Nelson (Oct 10 2020 at 14:55, on Zulip):

bjorn3 said:

By the way why does librustdoc actually exist separate from the rustdoc crate, when the rustdoc crate is just fn main() { rustdoc::main() }.

good question, I think it was something to do with compile times? @GuillaumeGomez might know

Joshua Nelson (Oct 10 2020 at 14:56, on Zulip):

bjorn3 said:

I want to override the codegen backend used by rustdoc without passing -Zcodegen-backend. I want an executable that passes an Box<dyn CodegenBackend> and a list of arguments to rustdoc.

my question is where rustdoc gets a CodegenBackend from at all

Joshua Nelson (Oct 10 2020 at 14:56, on Zulip):

like, where does it use it currently?

bjorn3 (Oct 10 2020 at 14:56, on Zulip):

rustc_interface has a default CodegenBackend that is cg_llvm.

bjorn3 (Oct 10 2020 at 14:57, on Zulip):

https://github.com/rust-lang/rust/blob/cae8bc1f2324e31c98cb32b8ed37032fc9cef405/compiler/rustc_interface/src/util.rs#L373

Joshua Nelson (Oct 10 2020 at 14:58, on Zulip):

ok, and why do you want to change that? I don't think rustdoc actually uses the codegen backend

(-bash@build-server) ~/rustc2 ▶️ rg get_codegen_backend src/librustdoc/ # no output
bjorn3 (Oct 10 2020 at 14:58, on Zulip):

It uses it for compiling tests.

Joshua Nelson (Oct 10 2020 at 14:58, on Zulip):

ahhh

Joshua Nelson (Oct 10 2020 at 14:58, on Zulip):

ok that's what I was missing

bjorn3 (Oct 10 2020 at 14:59, on Zulip):

get_codegen_backend is called by create_session by the way.

Joshua Nelson (Oct 10 2020 at 15:00, on Zulip):

ok I have a follow-up question: rustdoc uses all the arguments of rustc, why doesn't -Z codegen-backend work too?

Joshua Nelson (Oct 10 2020 at 15:01, on Zulip):

Joshua Nelson said:

ok I have a follow-up question: rustdoc uses all the arguments of rustc, why doesn't -Z codegen-backend work too?

err actually let me double check that

bjorn3 (Oct 10 2020 at 15:01, on Zulip):

It does work

Joshua Nelson (Oct 10 2020 at 15:02, on Zulip):

then I'm again confused what your question is

Joshua Nelson (Oct 10 2020 at 15:02, on Zulip):

are we not propagating the backend to doctests?

bjorn3 (Oct 10 2020 at 15:04, on Zulip):

In the current state doctests do work fine. I want to create an executable that passes -Ztrim-diagnostic-paths=no -Cpanic=abort -Zpanic-abort-tests -Zcodegen-backend='$(pwd)'/target/'$CHANNEL'/librustc_codegen_cranelift.'$dylib_ext' --sysroot '$(pwd)'/build_sysroot/sysroot to rustdoc. Where preferably -Zcodegen-backend is passing an Box<dyn CodegenBackend>. These are all the flags necessary for cg_clif. This is necessary to make using cg_clif a matter of RUSTC=cg_clif RUSTDOC=cg_clif_rustdoc cargo test.

bjorn3 (Oct 10 2020 at 15:05, on Zulip):

The RUSTC=cg_clif part already works. The RUSTDOC part doesn't. Instead you need that long list of arguments in RUSTDOCFLAGS.

Joshua Nelson (Oct 10 2020 at 15:06, on Zulip):

you want a way to do this programmatically instead of through CLI args

Joshua Nelson (Oct 10 2020 at 15:06, on Zulip):

ok, I understand now

bjorn3 (Oct 10 2020 at 15:06, on Zulip):

Yes

bjorn3 (Oct 10 2020 at 15:08, on Zulip):

And preferably with an arbitrary Box<dyn CodegenBackend> instead of requiring -Zcodegen-backend to be passed. That makes it possible to pass in extra information to the codegen backend. For example it may be used to enable JITing of tests instead of AOT compilation.

Joshua Nelson (Oct 10 2020 at 15:10, on Zulip):

it looks like the equivalent function for rustc is rustc_driver::run_compiler

Joshua Nelson (Oct 10 2020 at 15:10, on Zulip):

but rustdoc can't expose a function like that because it itself calls run_compiler

Joshua Nelson (Oct 10 2020 at 15:10, on Zulip):

maybe it could expose a wrapper around it?

bjorn3 (Oct 10 2020 at 15:11, on Zulip):

I think so. That would require adding librustdoc to the sysroot.

Joshua Nelson (Oct 10 2020 at 15:12, on Zulip):

I don't have any objections in principle, but I wouldn't know where to start with that

bjorn3 (Oct 10 2020 at 15:13, on Zulip):

I can try it.

Joshua Nelson (Oct 10 2020 at 15:13, on Zulip):

I'll give the rest of the team a heads-up

bjorn3 (Oct 10 2020 at 15:47, on Zulip):

I just realized that adding librustdoc to the sysroot will require compiling it again for every stage.

GuillaumeGomez (Oct 11 2020 at 15:01, on Zulip):

Joshua Nelson said:

bjorn3 said:

By the way why does librustdoc actually exist separate from the rustdoc crate, when the rustdoc crate is just fn main() { rustdoc::main() }.

good question, I think it was something to do with compile times? GuillaumeGomez might know

The idea is to allow to have different "front-end" to rustdoc in case you want to use librustdoc as a dependeny.

Joshua Nelson (Oct 11 2020 at 15:03, on Zulip):

I mean, in practice that's not reallly true because the only entry point is rustdoc::main

Joshua Nelson (Oct 11 2020 at 15:03, on Zulip):

so you have to take the exactly the same arguments as rustdoc

triagebot (Oct 14 2020 at 19:25, on Zulip):

This proposal has been accepted: #270.

bjorn3 (Oct 15 2020 at 09:08, on Zulip):

I initially planned to expose cg_clif using -Zcodegen-backend. This would require several changes to bootstrap and rustc_interface to compile and load it. Recently I made a custom driver for cg_clif. This has the advantage that it can be integrated as a regular tool with an interface similar to miri, but it means that bootstrap can't use cg_clif as backend to compile later stages. Which one should I use?

bjorn3 (Oct 15 2020 at 09:10, on Zulip):

What do you think @simulacrum, @oli?

oli (Oct 15 2020 at 10:53, on Zulip):

I would prefer codgen-backend, so we have a single API. I do realize the API is not well suited for backends other than LLVM, but I think the whole point was always to adjust it to needs as they come

bjorn3 (Oct 15 2020 at 11:15, on Zulip):

The -Zcodegen-backend api is suitable for other backends, but it needs rustc changes to load from the sysroot instead of just from a path. It also needs a bootstrap change to put it in the right place.

simulacrum (Oct 15 2020 at 11:33, on Zulip):

I think codegen-backend should be just fine. I'm not sure about the "loading from sysroot" bit - I recall intentionally removing some code there - but I think it should be fine. I'd not personally be opposed to linking rustc (optionally) directly with cranelift either

simulacrum (Oct 15 2020 at 11:33, on Zulip):

(so we don't need to ourselves go searching in various directories)

bjorn3 (Oct 15 2020 at 11:34, on Zulip):

Directly linking it in won't work. Cargo.toml doesn't contain references for all rustc_* crates, so it has to load them from sysroot, which means that it has to be a separate build step.

simulacrum (Oct 15 2020 at 11:42, on Zulip):

Hm, ok. I guess that's not something you want to change :)

I wish we already had support for = sysroot deps in cargo.toml, then we could do that sort of integration more easily.

bjorn3 (Oct 15 2020 at 12:30, on Zulip):

@simulacrum Can you take a look at https://github.com/rust-lang/rust/compare/master...bjorn3:cg_clif_subtree3

This should work with the exception of needing to disable lto (otherwise the linking code omits all object files), disable the nightly feature of parking_lot (inline asm using llvm_asm!) and pass -Ztrim-diagnostic-paths=no when compiling using cg_clif (#77458).

simulacrum (Oct 15 2020 at 12:35, on Zulip):

I will need to review in more detail, but nothing major seems wrong

bjorn3 (Oct 15 2020 at 12:39, on Zulip):

Should I also compile (not test) it on a CI builder? If so, which?

simulacrum (Oct 15 2020 at 12:43, on Zulip):

we should make sure x.py check runs for it

simulacrum (Oct 15 2020 at 12:44, on Zulip):

And if you think an actual build beyond that is helpful (I'm not actually sure) then tools builder is what I'd suggest

bjorn3 (Oct 15 2020 at 12:45, on Zulip):

Check should be enough. I don't do any crazy things that could fail during codegen.

bjorn3 (Oct 15 2020 at 13:18, on Zulip):

@simulacrum How do I set a config var only for a specific job?

simulacrum (Oct 15 2020 at 13:18, on Zulip):

Can you say more? What do you mean by "job"?

bjorn3 (Oct 15 2020 at 13:18, on Zulip):

A CI job

simulacrum (Oct 15 2020 at 13:18, on Zulip):

I would add it to the dockerfile

simulacrum (Oct 15 2020 at 13:19, on Zulip):

sr/cic/docker/host-x86_64/<name of ci builder>/Dockerfile

bjorn3 (Oct 15 2020 at 13:19, on Zulip):

I want to set rust.codegen-backends for the check job.

simulacrum (Oct 15 2020 at 13:19, on Zulip):

you'll probably want to do a RUN_CHECK_WITH_CODEGEN_BACKENDS 1 and follow the PARALLEL_QUERIES grep

bjorn3 (Oct 15 2020 at 13:27, on Zulip):

@simulacrum Does https://github.com/rust-lang/rust/commit/437058e1f0b52f689993eb345fb123c1a22f5685 look correct?

simulacrum (Oct 15 2020 at 13:34, on Zulip):

@bjorn3 I would move it up to where the main configure args are set, but yes seems fine

bjorn3 (Oct 15 2020 at 13:35, on Zulip):

Should I open a PR once I am done?

simulacrum (Oct 15 2020 at 13:36, on Zulip):

Yes, I think so.

bjorn3 (Oct 15 2020 at 13:47, on Zulip):

If CI is gated on cg_clif building, shouldn't it be compiled by default?

bjorn3 (Oct 15 2020 at 13:50, on Zulip):

@simulacrum ^

simulacrum (Oct 15 2020 at 13:51, on Zulip):

no, I don't think so

simulacrum (Oct 15 2020 at 13:51, on Zulip):

at least I presume that would add a good bit of compile times and seems not necessary

simulacrum (Oct 15 2020 at 13:51, on Zulip):

(for most contributors)

simulacrum (Oct 15 2020 at 13:51, on Zulip):

eventually maybe but not for now

simulacrum (Oct 15 2020 at 13:51, on Zulip):

x.py check should check it by default though

bjorn3 (Oct 15 2020 at 13:52, on Zulip):

It adds 2min for me when using ./x.py build.

simulacrum (Oct 15 2020 at 13:52, on Zulip):

out of?

simulacrum (Oct 15 2020 at 13:53, on Zulip):

but realistically that feels too long to be acceptable regression at this point

bjorn3 (Oct 15 2020 at 13:53, on Zulip):
Building stage0 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[...]
    Finished release [optimized] target(s) in 30m 46s
bjorn3 (Oct 15 2020 at 13:55, on Zulip):

I will make ./x.py check enable it by default.

simulacrum (Oct 15 2020 at 13:56, on Zulip):

hm out of 30 minutes I'd be less worried. not sure. I think we should leave it off by default initially anyway, maybe that'll change over time though

bjorn3 (Oct 15 2020 at 14:25, on Zulip):

On CI it adds 30s to ./x.py check. Most of this is only for the first build, as changing any rustc crate doesn't require a rebuild of Cranelift.

Josh Triplett (Oct 15 2020 at 21:43, on Zulip):

simulacrum said:

Hm, ok. I guess that's not something you want to change :)

I wish we already had support for = sysroot deps in cargo.toml, then we could do that sort of integration more easily.

We'd love to have that in Cargo; someone just needs to propose and write it.

Josh Triplett (Oct 15 2020 at 21:44, on Zulip):

(That's closely related to std-aware cargo; you'd probably want to coordinate it with @Eric Huss and others.)

simulacrum (Oct 15 2020 at 22:38, on Zulip):

:thumbs_up: I keep trying to find time for this but just haven't gotten cycles yet

Last update: May 07 2021 at 08:00UTC