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

Use lld with non-LLVM backends #132774

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Use lld with non-LLVM backends #132774

merged 1 commit into from
Nov 9, 2024

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Nov 8, 2024

On arm64, Cranelift used to produce object files that don't work with lld. This has since been fixed. The GCC backend should always produce object files that work with lld unless lld for whatever reason drops GCC support. Most of the other more niche backends don't use cg_ssa's linker code at all. If they do and don't work with lld, they can always disable lld usage using a cli argument.

Without this commit using cg_clif is by default in a non-trivial amount of cases a perf regression on Linux due to ld.bfd being a fair bit slower than lld. It is possible to explicitly enable it without this commit, but most users are unlikely to do this.

@bjorn3 bjorn3 added the A-cranelift Things relevant to the [future] cranelift backend label Nov 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Nov 8, 2024
@lqd
Copy link
Member

lqd commented Nov 8, 2024

Without this commit using cg_clif is in a non-trivial amount of cases a perf regression on Linux due to ld.bfd being a fair bit slower than lld.

Again, this is just when no flavor/self-contained-linking is specified.

r? lqd

@rustbot rustbot assigned lqd and unassigned jieyouxu Nov 8, 2024
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 8, 2024

I guess I should say "by default". Let me update the commit message.

@lqd
Copy link
Member

lqd commented Nov 8, 2024

There should be no reason CI would fail, but r=me when it's green.

On arm64, Cranelift used to produce object files that don't work with
lld. This has since been fixed. The GCC backend should always produce
object files that work with lld unless lld for whatever reason drops GCC
support. Most of the other more niche backends don't use cg_ssa's linker
code at all. If they do and don't work with lld, they can always disable
lld usage using a cli argument.

Without this commit using cg_clif is by default in a non-trivial amount
of cases a perf regression on Linux due to ld.bfd being a fair bit
slower than lld. It is possible to explicitly enable it without this
commit, but most users are unlikely to do this.
@bjorn3
Copy link
Member Author

bjorn3 commented Nov 9, 2024

@bors r=lqd

@bors
Copy link
Contributor

bors commented Nov 9, 2024

📌 Commit 6ffab47 has been approved by lqd

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

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do)
 - rust-lang#132363 (Enforce that raw lifetimes must be valid raw identifiers)
 - rust-lang#132744 (add regression test for rust-lang#90781)
 - rust-lang#132754 (Simplify the internal API for declaring command-line options)
 - rust-lang#132772 (use `download-rustc="if-unchanged"` as a global default)
 - rust-lang#132774 (Use lld with non-LLVM backends)
 - rust-lang#132799 (Make `Ty::primitive_symbol` recognize `str`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9b47807 into rust-lang:master Nov 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
Rollup merge of rust-lang#132774 - bjorn3:cranelift_lld, r=lqd

Use lld with non-LLVM backends

On arm64, Cranelift used to produce object files that don't work with lld. This has since been fixed. The GCC backend should always produce object files that work with lld unless lld for whatever reason drops GCC support. Most of the other more niche backends don't use cg_ssa's linker code at all. If they do and don't work with lld, they can always disable lld usage using a cli argument.

 Without this commit using cg_clif is by default in a non-trivial amount of cases a perf regression on Linux due to ld.bfd being a fair bit slower than lld. It is possible to explicitly enable it without this commit, but most users are unlikely to do this.
@bjorn3 bjorn3 deleted the cranelift_lld branch November 9, 2024 23:02
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Use lld with non-LLVM backends

On arm64, Cranelift used to produce object files that don't work with lld. This has since been fixed. The GCC backend should always produce object files that work with lld unless lld for whatever reason drops GCC support. Most of the other more niche backends don't use cg_ssa's linker code at all. If they do and don't work with lld, they can always disable lld usage using a cli argument.

 Without this commit using cg_clif is by default in a non-trivial amount of cases a perf regression on Linux due to ld.bfd being a fair bit slower than lld. It is possible to explicitly enable it without this commit, but most users are unlikely to do this.
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do)
 - rust-lang#132363 (Enforce that raw lifetimes must be valid raw identifiers)
 - rust-lang#132744 (add regression test for rust-lang#90781)
 - rust-lang#132754 (Simplify the internal API for declaring command-line options)
 - rust-lang#132772 (use `download-rustc="if-unchanged"` as a global default)
 - rust-lang#132774 (Use lld with non-LLVM backends)
 - rust-lang#132799 (Make `Ty::primitive_symbol` recognize `str`)

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

lqd commented Jan 9, 2025

The GCC backend should always produce object files that work with lld unless lld for whatever reason drops GCC support

This comment may not be completely accurate. @antoyo has been asking me weird obscure questions about lld for some reason for a few weeks now, as well as on zulip -- and I've only now realized that this PR of course initiated his issues, so I've referred him to your expertise @bjorn3.

I guess it may be interesting to only allow cg_clif to use a default lld for the time being, and not the other backends? At least until what I described in the comment this PR removes happens, some interactions with the backends may be warranted in some cases, and switching the linker unconditionally may be problematic. I really only wanted to switch lld with cg_llvm to prevent backend writers from encountering these kinds of unexpected issues.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 9, 2025

I did be fine with disabling lld for cg_gcc specifically and any other backend after they found issues until the root cause of those issues is found. I prefer not using an allowlist for using lld however as that will only delay discovery of lld related issues and when mixing multiple codegen backends it is still possible that lld gets used even if a backend isn't on the allowlist. Same with the end user adding rustflags = ["-Clink-arg=-fuse-ld=lld"] to their ~/.cargo/config.toml as I believe a fair amount of people do.

@antoyo
Copy link
Contributor

antoyo commented Jan 9, 2025

@bjorn3: For context, my last (hopefully) issue is that LTO doesn't work.
I presume that it's because it doesn't find the linker plugin and it seems lld doesn't have an option to set the linker plugin as mentioned in its doc:

--plugin= Ignored for compatibility with GNU linkers

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 9, 2025

Ah, right. Lld only supports LLVM LTO. Unless you use -Clinker-plugin-lto that shouldn't matter however. If -Clinker-plugin-lto is not used, the codegen backend is supposed to perform LTO itself (-Clto is a bit poorly named as it is not the linker that does the optimizations, but rustc) and emit object files containing machine code rather than LLVM bitcode or GIMPLE.

@antoyo
Copy link
Contributor

antoyo commented Jan 9, 2025

Unless you use -Clinker-plugin-lto that shouldn't matter however. If -Clinker-plugin-lto is not used, the codegen backend is supposed to perform LTO itself (-Clto is a bit poorly named as it is not the linker that does the optimizations, but rustc) and emit object files containing machine code rather than LLVM bitcode or GIMPLE.

Yeah, I was thinking about that: this must be what I'm doing wrong.

Does that mean that -Clinker-plugin-lto=<path> won't use the linker specified by path when using lld?

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 9, 2025

Yes, the path should be ignored with lld.

@antoyo
Copy link
Contributor

antoyo commented Jan 11, 2025

I was able to fix all the issues (well, I muted one for now 😄 ), so I guess we can keep lld as the default for the GCC codegen as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cranelift Things relevant to the [future] cranelift backend 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
Development

Successfully merging this pull request may close these issues.

6 participants