Reland `optimized-compiler-builtins` config #102579

jyn514 · 2022-10-02T13:23:31Z

this is possible now that compiler-builtins contains the fix in rust-lang/compiler-builtins#532.

in particular, this makes the c feature for compiler-builtins an explicit opt-in, rather than silently detected by whether llvm-project is checked out on disk.
exposing this is necessary because the cc crate doesn't support cross-compiling to MSVC, and we want people to be able to run x check --target foo regardless of whether they have a c toolchain available.

this also uses the new option in CI, where we do want to optimize compiler_builtins.

the new option is off by default for the dev channel and on otherwise.

Fixes #102560. Fixes #101172. Helps with #105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

r? @tmandry cc @Mark-Simulacrum

tmandry · 2022-10-02T19:01:59Z

src/bootstrap/lib.rs

    fn is_rust_llvm(&self, target: TargetSelection) -> bool {
        match self.config.target_config.get(&target) {
+            // We're using a pre-built version of LLVM, but the user has promised that pre-built version has our patches.


We may not be using a pre-built version of LLVM. Specifically the use case I want to support is cd into src/llvm-project, check out a vanilla commit, and set submodules = false, llvm-has-rust-patches = false in config.

That seems totally bizarre, though. Why would you want that?

To build against a more recent version of LLVM.

If you separately build and supply your own llvm-config, then you hit the other assumptions in here about not needing to distribute the LLVM libraries because it's probably system installed. So basically this was unsupported before.

Plus it's desirable to have bootstrap manage the configuration of LLVM if all you want to vary is the version.

tmandry · 2022-10-02T19:07:15Z

src/bootstrap/compile.rs

@@ -313,12 +313,17 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
    let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins {
        if !builder.is_rust_llvm(target) {


You can use compiler-rt without Rust patches. I think the important thing is that compiler-rt is built from the same sources as LLVM. So.. either llvm-config is unset, or llvm_from_ci is active and we're managing submodules?

Yeah, as I wrote in #101833 (comment) enabling optimized_compiler_builtins is kinda required on some targets, and from my understanding the only requirement is that the sources of compiler-rt has to be the same as the custom LLVM's. We probably need a way to supply the custom sources in those cases.

tmandry · 2022-10-18T21:58:42Z

@rustbot author

…iltins, r=jyn514 Revert "Make the `c` feature for `compiler-builtins` opt-in instead of inferred" This reverts commit 3acb505 (PR rust-lang#101833). The changes in this commit caused several bugs/incompatibilities (rust-lang#101833 (comment), rust-lang#102560). For now we're reverting this commit and will re-land it alongside fixes for those bugs. Re-opens rust-lang#101172 cc rust-lang#102560 cc rust-lang#102579

bors · 2022-10-31T16:20:41Z

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

jyn514 · 2022-11-25T00:24:44Z

@tmandry I am still very confused how llvm_has_rust_patches is meant to be used. The only place I see it used is in Build::is_rust_llvm, which is used in three places:

rust/src/bootstrap/test.rs

Lines 1553 to 1554 in c5f92ce

if !builder.is_rust_llvm(target) {

cmd.arg("--system-llvm");
The new assert I just added. Let's ignore that for now.
rust/src/bootstrap/compile.rs

Lines 805 to 806 in b3bc6bf

if builder.is_rust_llvm(target) {

cargo.env("LLVM_RUSTLLVM", "1");

3 defines LLVM_RUSTLLVM, which is eventually used in

rust/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp

Lines 308 to 311 in 693c631

    
           #ifdef LLVM_RUSTLLVM 
        
           /// getLongestEntryLength - Return the length of the longest entry in the table. 
        
           template<typename KV> 
        
           static size_t getLongestEntryLength(ArrayRef<KV> Table) {

; that bit looks correct. But 1. looks absolutely wrong even before this PR: it will pass --system-llvm for an in-tree commit with submodules = false and patches = false. How is llvm_has_rust_patches tested? how is it supposed to work? I ran llvm-config --ldflags --system-llvm locally and it didn't even exit successfully ...

jyn514 · 2022-11-25T00:29:27Z

ugh I misread, it's a compiletest flag, not an llvm-config flag. so I think what was happening before is that some tests were silently being ignored, which we don't have any sort of checks for:

rust/src/tools/compiletest/src/header.rs

Lines 1059 to 1061 in d5fd1af

    
           fn ignore_llvm(config: &Config, line: &str) -> bool { 
        
               if config.system_llvm && line.starts_with("no-system-llvm") { 
        
                   return true;

I'll fix 1 at the same time as I address your and pietro's comments.

jyn514 · 2022-11-25T01:05:58Z

Ok, pushed a few changes. @pietroalbini @tmandry let me know if you think this will work better.

config.toml.example

src/bootstrap/compile.rs

src/ci/docker/host-x86_64/x86_64-gnu-llvm-13/Dockerfile

tmandry · 2023-01-25T21:12:18Z

@rustbot author (I think there are some outstanding concerns from Pietro to be addressed)

tmandry

Looks good, thanks for checking! I left some nits, feel free to fix or ignore and r=me

src/bootstrap/lib.rs

bors · 2023-07-29T16:54:20Z

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

jyn514 · 2023-08-21T15:42:12Z

rust-lang/team#1051

rustbot · 2023-12-25T12:50:11Z

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

jyn514 · 2023-12-25T13:07:52Z

i am going to remove the check that the in-tree llvm matches the llvm we're linking to, actually. i am not sure the difference matters, and it wasn't enforced before.

in particular, this makes the `c` feature for compiler-builtins an explicit opt-in, rather than silently detected by whether `llvm-project` is checked out on disk. exposing this is necessary because the `cc` crate doesn't support cross-compiling to MSVC, and we want people to be able to run `x check --target foo` regardless of whether they have a c toolchain available. this also uses the new option in CI, where we *do* want to optimize compiler_builtins. the new option is off by default for the `dev` channel and on otherwise.

jyn514 · 2023-12-25T13:17:41Z

r? bootstrap

jyn514 · 2023-12-25T13:17:52Z

@rustbot ready

bors · 2023-12-29T19:37:36Z

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

onur-ozkan

No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

…tins, r=onur-ozkan Reland optimized-compiler-builtins config Copy of rust-lang#102579 PR. From rust-lang#102579: > No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it. > Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true). Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 r? ghost

…ns, r=onur-ozkan Reland optimized-compiler-builtins config Copy of rust-lang#102579 PR. From rust-lang#102579: > No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it. > Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true). Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 r? ghost

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 2, 2022

rust-highfive assigned tmandry Oct 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2022

This comment has been minimized.

Sign in to view

jyn514 force-pushed the fix-llvm-handling branch 2 times, most recently from fbb4c86 to 37c21a3 Compare October 2, 2022 13:50

tmandry reviewed Oct 2, 2022

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2022

Mark-Simulacrum mentioned this pull request Oct 29, 2022

Revert "Make the c feature for compiler-builtins opt-in instead of inferred" #103732

Merged

tmandry removed their assignment Nov 14, 2022

jyn514 assigned tmandry Nov 25, 2022

jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 25, 2022

jyn514 force-pushed the fix-llvm-handling branch from 37c21a3 to 7945458 Compare November 25, 2022 00:27

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 25, 2022

This comment has been minimized.

Sign in to view

jyn514 force-pushed the fix-llvm-handling branch from 50baed3 to a8b210d Compare November 25, 2022 01:05

jyn514 assigned pietroalbini Nov 25, 2022

pietroalbini requested changes Nov 27, 2022

View reviewed changes

config.toml.example Outdated Show resolved Hide resolved

src/bootstrap/compile.rs Outdated Show resolved Hide resolved

src/ci/docker/host-x86_64/x86_64-gnu-llvm-13/Dockerfile Outdated Show resolved Hide resolved

mkroening mentioned this pull request Nov 29, 2022

Wrong architecture objects mixed in self-built compiler #105065

Open

jyn514 removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 8, 2023

jyn514 mentioned this pull request Jul 17, 2023

Check tier 3 targets in CI #109099

Open

tmandry approved these changes Jul 18, 2023

View reviewed changes

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

src/bootstrap/lib.rs Outdated Show resolved Hide resolved

Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2023

jyn514 closed this Aug 21, 2023

jyn514 reopened this Dec 25, 2023

add and document a new is_system_llvm abstraction

b3ebe50

jyn514 force-pushed the fix-llvm-handling branch from 38392fb to eb945e2 Compare December 25, 2023 12:50

This comment has been minimized.

Sign in to view

jyn514 force-pushed the fix-llvm-handling branch from eb945e2 to a10b1be Compare December 25, 2023 13:15

rustbot assigned onur-ozkan and unassigned tmandry Dec 25, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 25, 2023

onur-ozkan approved these changes Jan 3, 2024

View reviewed changes

onur-ozkan closed this Jan 3, 2024

onur-ozkan mentioned this pull request Jan 3, 2024

Reland optimized-compiler-builtins config #119556

Merged

jyn514 deleted the fix-llvm-handling branch January 17, 2024 16:28

onur-ozkan removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland `optimized-compiler-builtins` config #102579

Reland `optimized-compiler-builtins` config #102579

jyn514 commented Oct 2, 2022 •

edited

Loading

This comment has been minimized.

tmandry Oct 2, 2022

jyn514 Oct 2, 2022

tmandry Oct 2, 2022

tmandry Oct 2, 2022

pietroalbini Oct 6, 2022

tmandry commented Oct 18, 2022

bors commented Oct 31, 2022

jyn514 commented Nov 25, 2022 •

edited

Loading

jyn514 commented Nov 25, 2022 •

edited

Loading

This comment has been minimized.

jyn514 commented Nov 25, 2022

tmandry commented Jan 25, 2023

tmandry left a comment

bors commented Jul 29, 2023

jyn514 commented Aug 21, 2023

rustbot commented Dec 25, 2023

This comment has been minimized.

jyn514 commented Dec 25, 2023

jyn514 commented Dec 25, 2023

jyn514 commented Dec 25, 2023

bors commented Dec 29, 2023

onur-ozkan left a comment

		@@ -313,12 +313,17 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
		let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins {
		if !builder.is_rust_llvm(target) {

Reland optimized-compiler-builtins config #102579

Reland optimized-compiler-builtins config #102579

Conversation

jyn514 commented Oct 2, 2022 • edited Loading

This comment has been minimized.

tmandry Oct 2, 2022

Choose a reason for hiding this comment

jyn514 Oct 2, 2022

Choose a reason for hiding this comment

tmandry Oct 2, 2022

Choose a reason for hiding this comment

tmandry Oct 2, 2022

Choose a reason for hiding this comment

pietroalbini Oct 6, 2022

Choose a reason for hiding this comment

tmandry commented Oct 18, 2022

bors commented Oct 31, 2022

jyn514 commented Nov 25, 2022 • edited Loading

jyn514 commented Nov 25, 2022 • edited Loading

This comment has been minimized.

jyn514 commented Nov 25, 2022

tmandry commented Jan 25, 2023

tmandry left a comment

Choose a reason for hiding this comment

bors commented Jul 29, 2023

jyn514 commented Aug 21, 2023

rustbot commented Dec 25, 2023

This comment has been minimized.

jyn514 commented Dec 25, 2023

jyn514 commented Dec 25, 2023

jyn514 commented Dec 25, 2023

bors commented Dec 29, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

Reland `optimized-compiler-builtins` config #102579

Reland `optimized-compiler-builtins` config #102579

jyn514 commented Oct 2, 2022 •

edited

Loading

jyn514 commented Nov 25, 2022 •

edited

Loading

jyn514 commented Nov 25, 2022 •

edited

Loading