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

Reland optimized-compiler-builtins config #119556

Merged
merged 2 commits into from
Jan 7, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jan 3, 2024

Copy of #102579 PR.

From #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 #102560. Fixes #101172. Helps with #105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes #102560. Fixes #101172. Helps with #105065

r? ghost

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 3, 2024

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.

@onur-ozkan
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Jan 3, 2024

📌 Commit b5ccbcb has been approved by onur-ozkan

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 Jan 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 4, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#117636 (add test for rust-lang#117626)
 - rust-lang#118704 (Promote `riscv32{im|imafc}` targets to tier 2)
 - rust-lang#119184 (Switch from using `//~ERROR` annotations with `--error-format` to `error-pattern`)
 - rust-lang#119325 (custom mir: make it clear what the return block is)
 - rust-lang#119391 (Use Result::flatten in catch_with_exit_code)
 - rust-lang#119431 (Support reg_addr register class in s390x inline assembly)
 - rust-lang#119475 (Remove libtest's dylib)
 - rust-lang#119532 (Make offset_of field parsing use metavariable which handles any spacing)
 - rust-lang#119553 (stop feed vis when cant access for trait item)
 - rust-lang#119556 (Reland optimized-compiler-builtins config)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=never
looks like it failed in https://github.com/rust-lang/rust/pulls#issuecomment-1877189661

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 4, 2024
if [ "$EXTERNAL_LLVM" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.optimized-compiler-builtins"
elif [ "$DEPLOY$DEPLOY_ALT" = "1" ]; then
echo "error: dist builds should always use optimized compiler-rt!" >&2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dist-various-2 image uses an external LLVM and fails here. I am considering to update this image to not install/use the external LLVM and instead opt for compiling and using the in-tree one like we do in the dist-various-1 image. Before doing it, I want to know if @rust-lang/infra team is happy with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does dist-various-2 really use an external LLVM? I don't see llvm-root/llvm-config options in the Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't use, my bad. I got confused with "ENV EXTERNAL_LLVM 1"

@@ -106,4 +106,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
severity: ChangeSeverity::Info,
summary: "The dist.missing-tools config option was deprecated, as it was unused. If you are using it, remove it from your config, it will be removed soon.",
},
ChangeInfo {
change_id: 102579,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you didn't update the PR number

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the actual PR number of the implementation.

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.
@onur-ozkan
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2024

📌 Commit 6a409dd has been approved by onur-ozkan

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 7, 2024
@bors
Copy link
Contributor

bors commented Jan 7, 2024

⌛ Testing commit 6a409dd with merge 87e1430...

@bors
Copy link
Contributor

bors commented Jan 7, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 87e1430 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2024
@bors bors merged commit 87e1430 into rust-lang:master Jan 7, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 7, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (87e1430): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.2% [3.9%, 4.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-3.0%, -2.1%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.625s -> 665.728s (-0.43%)
Artifact size: 308.39 MiB -> 308.41 MiB (0.00%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2024
…ozkan

Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.

This adapts an assertion that was added in rust-lang#119556.

The current assertion does not take the `llvm-config` setting into accounts, which does not match the `llvm-has-rust-patches` documentation, which states:

> This would be used in conjunction with either an llvm-config or build.submodules = false.

(It also breaks my workflow: I build LLVM separately, but do have the rust patches applied).

---

**edit:** Originally this PR just removed the assertion, but it now implements the alternative mentioned here:

An alternative fix would be to take `llvm-config` into account in the assertion, but to me the assertion seems to provide little value, thus the simpler fix of just removing it.

cc `@onur-ozkan,` in case I'm missing a reason to keep the assertion.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024
Rollup merge of rust-lang#120890 - TimNN:relax-patches-check, r=onur-ozkan

Adapt `llvm-has-rust-patches` validation to take `llvm-config` into account.

This adapts an assertion that was added in rust-lang#119556.

The current assertion does not take the `llvm-config` setting into accounts, which does not match the `llvm-has-rust-patches` documentation, which states:

> This would be used in conjunction with either an llvm-config or build.submodules = false.

(It also breaks my workflow: I build LLVM separately, but do have the rust patches applied).

---

**edit:** Originally this PR just removed the assertion, but it now implements the alternative mentioned here:

An alternative fix would be to take `llvm-config` into account in the assertion, but to me the assertion seems to provide little value, thus the simpler fix of just removing it.

cc `@onur-ozkan,` in case I'm missing a reason to keep the assertion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
8 participants