Extract some shared code from codegen backend target feature handling by RalfJung · Pull Request #140920 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation64 Commits6 Checks18 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

@RalfJung

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in #135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

@rustbot

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

May 11, 2025

RalfJung

Choose a reason for hiding this comment

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

There's a lot more logic in this file that seems like it should be shared across backends (almost all of fn codegen_fn_attrs), but I shied away from the huge refactor that would have been.

@rust-log-analyzer

This comment has been minimized.

@RalfJung

Turns out properly adding negative implications to the target_features we set for LLVM tends to add a lot of target features -- we have some quite extensive implication chains. Is that a problem? Cc @nikic @taiki-e

@rust-log-analyzer

This comment has been minimized.

@antoyo

if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not?

There is no reason for that. I probably missed some stuff when I implemented this.
I'd be happy to have the same behavior as LLVM here.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@nikic

Turns out properly adding negative implications to the target_features we set for LLVM tends to add a lot of target features -- we have some quite extensive implication chains. Is that a problem? Cc @nikic @taiki-e

Anything in particular you're concerned about? But generally, no, it shouldn't be a problem.

@RalfJung

I can imagine two concerns:

@bors

@compiler-errors

@bors

@RalfJung

@nnethercote

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions.

That sounds reasonable.

I placed those in rustc_middle since I couldn't think of a better place to put them... I'm open for suggestions. :)

I think you've gotten off-track here.

rustc_codegen_ssa is the logical spot for stuff shared between backends, because all backends depend on it. The doc comment at the top of its lib.rs says //! This crate contains codegen code that is used by all codegen backends (LLVM and others).

Also, rustc_middle is already too big and shouldn't get anything added to it that doesn't need to be.

Here's part of the crate graph. You can see how rustc_codegen_llvm depends directly on rustc_codegen_ssa, while rustc_middle is several layers further down. rustc_codegen_gcc isn't included, I think because it's in a sub-repo, but it would sit next to rustc_codegen_llvm and also depend directly on rustc_codegen_ssa.

Screenshot from 2025-05-29 19-08-49

Crate graph generated with:

cargo +nightly depgraph --all-deps --dedup-transitive-deps --workspace-only > ~/graph.dot;
dot -Tpng ~/graph.dot > ~/graph.png

Based on this, I think significant parts of this PR are unnecessary:

This will significantly reduce the size of the PR, e.g. the entire first commit would disappear, along with part of the third commit.

@bors

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

Jun 18, 2025

@bors

…iaskrgr

Rollup of 9 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various

WaffleLapkin

Choose a reason for hiding this comment

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

r=nnethercote,WaffleLapkin after conflict resolution

@RalfJung

@RalfJung

@RalfJung

This does change the logic a bit: previously, we didn't forward reverse implications of negated features to the backend, instead relying on the backend to handle the implication itself.

@RalfJung

@RalfJung

@RalfJung

@bors r=nnethercote,WaffleLapkin

@bors

📌 Commit 0c4b0f5 has been approved by nnethercote,WaffleLapkin

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

Jun 19, 2025

@RalfJung

@RalfJung

I've merged #142500 into this, there's no longer a reason to have them separate...

@RalfJung

@bors r=nnethercote,WaffleLapkin

@bors

📌 Commit a50a3b8 has been approved by nnethercote,WaffleLapkin

It is now in the queue for this repository.

tgross35 added a commit to tgross35/rust that referenced this pull request

Jun 20, 2025

@tgross35

…n, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in rust-lang#135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

bors added a commit that referenced this pull request

Jun 20, 2025

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit that referenced this pull request

Jun 20, 2025

@bors

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Jun 20, 2025

@rust-timer

Rollup merge of #140920 - RalfJung:target-feature-unification, r=nnethercote,WaffleLapkin

Extract some shared code from codegen backend target feature handling

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not? The second commit also cleans up a bunch of unneeded complexity added in #135927.

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

The fourth commit increases consistency of the GCC backend with the LLVM backend.

The last commit does some further cleanup:

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

@RalfJung RalfJung deleted the target-feature-unification branch

June 21, 2025 10:26

antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request

Jun 28, 2025

@bors

Labels

A-attributes

Area: Attributes (`#[…]`, `#![…]`)

A-LLVM

Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.

A-rustdoc-json

Area: Rustdoc JSON 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.