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.

Sign up for GitHub

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

Jump to bottom

Allow use of -Clto=thin with -Ccodegen-units=1 in general #103610

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Allow use of -Clto=thin with -Ccodegen-units=1 in general #103610

merged 1 commit into from
Nov 3, 2022

Conversation

wesleywiser
Copy link
Member

The current logic to ignore ThinLTO when -Ccodegen-units=1 makes sense for local ThinLTO but even in this scenario, a user may still want (non-local) ThinLTO for the purpose of optimizing dependencies into the final crate which is being compiled with 1 CGU.

The previous behavior was even more confusing because if you were generating a binary (--emit=link), then you would get ThinLTO but if you asked for LLVM IR or bytecode, then it would silently change to using regular LTO.

With this change, we only override the defaults for local ThinLTO if you ask for a single output such as LLVM IR or bytecode and in all other cases honor the requested LTO setting.

r? @michaelwoerister

@wesleywiser
Allow use of -Clto=thin with -Ccodegen-units=1 in general
7c6345d 
The current logic to ignore ThinLTO when `-Ccodegen-units=1` makes sense
for local ThinLTO but even in this scenario, a user may still want
(non-local) ThinLTO for the purpose of optimizing dependencies into the
final crate which is being compiled with 1 CGU.

The previous behavior was even more confusing because if you were
generating a binary (`--emit=link`), then you would get ThinLTO but if
you asked for LLVM IR or bytecode, then it would silently change to
using regular LTO.

With this change, we only override the defaults for local ThinLTO if you
ask for a single output such as LLVM IR or bytecode and in all other
cases honor the requested LTO setting.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 27, 2022
@michaelwoerister
Copy link
Member

I originally wanted to do a crater run for this but now I'm wondering if crater could even find anything here, given that the change only affects --emit options that are not used by Cargo (i.e. llvm-ir, llvm-bitcode, asm, obj)...

But let's do one anyway, to be on the safe side: @bors try

@bors
Copy link
Contributor

bors commented Oct 27, 2022

⌛ Trying commit 7c6345d with merge 14accda7ba2c586835d15379a621955d9f047371...

@bors
Copy link
Contributor

bors commented Oct 27, 2022

☀️ Try build successful - checks-actions
Build commit: 14accda7ba2c586835d15379a621955d9f047371 (14accda7ba2c586835d15379a621955d9f047371)

@wesleywiser

This comment was marked as outdated.

Sign in to view
@craterbot

This comment was marked as outdated.

Sign in to view
@wesleywiser

This comment was marked as outdated.

Sign in to view
@craterbot

This comment was marked as outdated.

Sign in to view
@wesleywiser
Copy link
Member Author

@craterbot run mode=build-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-103610 created and queued.
🤖 Automatically detected try build 14accda7ba2c586835d15379a621955d9f047371
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-103610 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-103610 is completed!
📊 57 regressed and 22 fixed (246657 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 28, 2022
@wesleywiser
Copy link
Member Author

I looked through the crater results and didn't see anything very concerning. Most (probably like 75%) of the failures are because the node ran out of disk space during the build and there are lot of various error messages from rustc/llvm/ld failing due to IO errors. The next most common error was an older version of the libc crate failing to parse rustc's version (probably because of the -nightly suffix). There were a few crates that failed to build with Rust errors but that doesn't seem related to this change.

Finally, there was one crate that failed to link because of duplicate symbols (https://github.com/lann/code-snippets). The crate appears to indirectly depend on multiple versions of wit_bindgen_rust which exposes a #[no_mangle] function (canonical_abi_free). For whatever reason, the crate is able to link on nightly but fails with the try build. A more recent version of the wit_bindgen_rust crate has removed the #[no_mangle] attribute so if the dependencies are updated, it should work fine.

I can try to investigate further if we think that is warranted.

@michaelwoerister
Copy link
Member

I can't find anything in either of those projects that looks like it would use the features in question. I don't think the build failure is related to this PR.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 31, 2022

📌 Commit 7c6345d has been approved by michaelwoerister

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 Oct 31, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 2, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 3, 2022
@bors
Auto merge of rust-lang#103903 - matthiaskrgr:rollup-r5xcvrp, r=matth…
96787c4 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#99801 (fix(generic_const_exprs): Fix predicate inheritance for children of opaque types)
 - rust-lang#103610 (Allow use of `-Clto=thin` with `-Ccodegen-units=1` in general)
 - rust-lang#103870 (Fix `inferred_kind` ICE)
 - rust-lang#103875 (Simplify astconv item def id handling)
 - rust-lang#103886 (rustdoc: Fix merge of attributes for reexports of local items)
 - rust-lang#103890 (rustdoc: remove unused mobile CSS `.rustdoc { padding-top: 0 }`)

Failed merges:

 - rust-lang#103884 (Add visit_fn_ret_ty to hir intravisit)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0f72a6d into rust-lang:master Nov 3, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 3, 2022
@wesleywiser wesleywiser mentioned this pull request Nov 3, 2022
Merged
@pacak pacak mentioned this pull request Mar 9, 2023
Closed
pacak added a commit to pacak/cargo-show-asm that referenced this pull request Mar 9, 2023
@pacak
override lto setting
d87bca9 
cargo-show-asm relies on --emit to tell it which file was generated,
In rustc past rust-lang/rust#103610 this no
longer works even with -Ccodegen-units=1

This should not affect c-s-a's output since it wasn't looking at
linker's optimization but might affect how compiled program performs.
@pacak
Copy link
Contributor

pacak commented Mar 9, 2023

FWIW this broke cargo-show-asm for binary crates with lto=thin, see pacak/cargo-show-asm#146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wesleywiser @michaelwoerister @bors @craterbot @pacak @rustbot