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

Make the "codegen" profile of config.toml download and build llvm from source. #110074

Merged
merged 2 commits into from
Apr 9, 2023

Conversation

majaha
Copy link
Contributor

@majaha majaha commented Apr 8, 2023

The stated purpose of the codegen profile in config.toml is:

# These defaults are meant for contributors to the compiler who modify codegen or LLVM

but download-ci-llvm must be set to be false for the llvm source to even be downloaded. This patch adds that in.

Also included: a small docs fix in config.example.toml

This commit:
fcb2a36 (Rename `config.toml.example` to `config.example.toml`, 2023-03-11)
missed an instance in `config.example.toml` itself.
@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2023

This PR changes src/bootstrap/defaults/config.codegen.toml. If appropriate, please also update config.compiler.toml so the defaults are in sync.

@jyn514
Copy link
Member

jyn514 commented Apr 8, 2023

If we're going to make changes here, I'd prefer to implement support for if-unchanged instead, like how download-rustc works. Then if LLVM is modified locally it will behave as expected, but people who don't modify LLVM will continue to have fast compile times.

That said, it looks like I accidentally regressed this in #104512, so I don't mind merging your PR as-is and opening a follow-up issue - let me know if you'd prefer that.

@majaha
Copy link
Contributor Author

majaha commented Apr 8, 2023

That's interesting. I have some thoughts:

  1. ...but people who don't modify LLVM will continue to have fast compile times.

Isn't this what profile = "compiler" is for? Or does that not adequately meet the needs of people who are writing HIR and MIR? Do we need to split the codegen profile into an llvm profile and an HIR/MIR profile? We might not need the complicating parts of (2.) if we did.

  1. The ci-llvm version of llvm is missing a significant number of tools that you get with a source build. Tools like llc and opt are useful for running, checking, and debugging llvm by itself. Here's a comparison:
Source built llvm matt@Matts-PC:~/rust/build.old/x86_64-unknown-linux-gnu/llvm/bin$ ls FileCheck llvm-cxxmap llvm-lto2 llvm-sim UnicodeNameMappingGenerator llvm-debuginfo-analyzer llvm-mc llvm-size bugpoint llvm-debuginfod llvm-mca llvm-split count llvm-debuginfod-find llvm-ml llvm-stress dsymutil llvm-diff llvm-modextract llvm-strings llc llvm-dis llvm-mt llvm-strip lli llvm-dlltool llvm-nm llvm-symbolizer lli-child-target llvm-dwarfdump llvm-objcopy llvm-tapi-diff llvm-PerfectShuffle llvm-dwarfutil llvm-objdump llvm-tblgen llvm-addr2line llvm-dwp llvm-opt-report llvm-tli-checker llvm-ar llvm-exegesis llvm-otool llvm-undname llvm-as llvm-extract llvm-pdbutil llvm-windres llvm-bcanalyzer llvm-gsymutil llvm-profdata llvm-xray llvm-bitcode-strip llvm-ifs llvm-profgen not llvm-c-test llvm-install-name-tool llvm-ranlib obj2yaml llvm-cat llvm-jitlink llvm-rc opt llvm-cfi-verify llvm-jitlink-executor llvm-readelf sancov llvm-config llvm-lib llvm-readobj sanstats llvm-cov llvm-libtool-darwin llvm-reduce split-file llvm-cvtres llvm-link llvm-remark-size-diff verify-uselistorder llvm-cxxdump llvm-lipo llvm-remarkutil yaml-bench llvm-cxxfilt llvm-lto llvm-rtdyld yaml2obj
ci-llvm matt@Matts-PC:~/rust/build/x86_64-unknown-linux-gnu/ci-llvm/bin$ ls FileCheck llvm-ar llvm-config llvm-dis llvm-dwp llvm-objdump llvm-tblgen lld llvm-bcanalyzer llvm-cov llvm-dwarfdump llvm-nm llvm-profdata

Although I do notice that lld is conspicuously missing from the source-built version. I don't know why that is, or if it matters.

The downloadable llvm for this profile should probably include all the build artefacts that you get with a source build - so that the build dir is left in a state as if you've just run a full source build. It's not clear how you could prompt these to build otherwise if you had download-ci-llvm = if-unchanged set, short of making some arbitrary changes to the source files.

  1. At the moment, after an ordinary git clone, the llvm-project git submodule isn't initialised until you run ./x.py build with download-ci-llvm = false in your config.toml. This means that the llvm source code isn't there for reading or editing yet and will have to be downloaded, which somewhat defeats the purpose of your if-unchanged idea. I think the submodule should be downloaded during ./x.py setup after you select the codegen profile.

I think for now we should merge this as-is so that new contributors aren't left confused like I was. (It's Google Summer of Code soon!). And what I suggested in (1.) should be investigated first as the simplest possible solution.

@jyn514
Copy link
Member

jyn514 commented Apr 8, 2023

Isn't this what profile = "compiler" is for? Or does that not adequately meet the needs of people who are writing HIR and MIR? Do we need to split the codegen profile into an llvm profile and an HIR/MIR profile? We might not need the complicating parts of (2.) if we did.

I really would rather not add more profiles. People already get confused about compiler vs library when they make changes to stage 1 std that require compiler changes.

The group I'm thinking of for "touches codegen but not LLVM" is anyone who touches rustc_codegen_ssa or rustc_codegen_llvm; i.e. they're only changing the IR we send to LLVM, not LLVM itself. We should keep llvm.debug-assertions on for them but not for profile = "compiler".

Although I do notice that lld is conspicuously missing from the source-built version. I don't know why that is, or if it matters.

This is configured by llvm.lld:

rust/config.example.toml

Lines 606 to 608 in f98a271

# Indicates whether LLD will be compiled and made available in the sysroot for
# rustc to execute.
#lld = false

The downloadable llvm for this profile should probably include all the build artefacts that you get with a source build - so that the build dir is left in a state as if you've just run a full source build.

👍 makes sense, I agree we should make that change before changing the codegen profile to use if-unchanged by default. I don't think it should block adding if-unchanged as an option, though.

3. I think the submodule should be downloaded during ./x.py setup after you select the codegen profile.

👍 this seems reasonable, I would expect most people touching rustc_codegen_llvm to want to read through llvm-project even if they're not editing it. Doing it automatically also has the advantage that bootstrap uses shallow clones for submodules, I think a lot of people aren't aware of shallow clones.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 8, 2023

📌 Commit ee554e2 has been approved by jyn514

It is now in the queue for this repository.

@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 Apr 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#110058 (Remove `box_syntax` usage)
 - rust-lang#110059 (ignore_git → omit_git_hash)
 - rust-lang#110060 (Document that `&T` and `&mut T` are `Sync` if `T` is)
 - rust-lang#110074 (Make the "codegen" profile of `config.toml` download and build llvm from source.)
 - rust-lang#110086 (Add `max_line_length` to `.editorconfig`, matching rustfmt)
 - rust-lang#110096 (Tweak tuple indexing suggestion)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 66b3ad4 into rust-lang:master Apr 9, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 9, 2023
@majaha majaha deleted the config_toml_fix branch April 8, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants