Extract some shared code from codegen backend target feature handling by RalfJung · Pull Request #140920 · rust-lang/rust
rustbot
added
the
T-compiler
label
May 11, 2025This 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.
bors
added
S-waiting-on-bors
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.labels
Jun 19, 2025tgross35 added a commit to tgross35/rust that referenced this pull request
Jun 20, 2025…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: - Before, with `-Ctarget-feature=-feat`, if there is some other feature `x` that implies `feat` we would *not* add `-x` to the backend target feature set. Now, we do. This fixes rust-lang#134792. - The logic that removes `x` from `cfg(target_feature)` in this case also changed a bit, avoiding a large number of calls to the (uncached) `sess.target.implied_target_features` (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before `tcx` so we can't use that... - Previously, if feature "a" implied "b" and "b" was unstable, then using `-Ctarget-feature=+a` would also emit a warning about `b`. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway. The fourth commit increases consistency of the GCC backend with the LLVM backend. The last commit does some further cleanup: - Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes rust-lang#142412. - Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of `-Ctarget-feature` that only care about actual target features (and not "crt-static") have it. Previously, we actually set `cfg(target_feature = "crt-static")` twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore `-Ctarget-feature=+crt-static`, it seems like before this PR that flag still incorrectly enabled `cfg(target_feature = "crt-static")` (but I didn't test this). - Move fixed_x18 handling together with retpoline handling. - Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the `-Z` flag. `@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, 2025Rollup of 8 pull requests Successful merges: - #138291 (rewrite `optimize` attribute to use new attribute parsing infrastructure) - #140920 (Extract some shared code from codegen backend target feature handling) - #141990 (Implement send_signal for unix child processes) - #142597 (error on calls to ABIs that cannot be called) - #142668 (vec_deque/fmt/vec tests: remove static mut) - #142687 (Reduce uses of `hir_crate`.) - #142699 (Update books) - #142714 (add comment to `src/bootstrap/build.rs`) r? `@ghost` `@rustbot` modify labels: rollup
bors added a commit that referenced this pull request
Jun 20, 2025Rollup of 8 pull requests Successful merges: - #138291 (rewrite `optimize` attribute to use new attribute parsing infrastructure) - #140920 (Extract some shared code from codegen backend target feature handling) - #141990 (Implement send_signal for unix child processes) - #142668 (vec_deque/fmt/vec tests: remove static mut) - #142687 (Reduce uses of `hir_crate`.) - #142699 (Update books) - #142714 (add comment to `src/bootstrap/build.rs`) - #142753 (Update library dependencies) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer added a commit that referenced this pull request
Jun 20, 2025Rollup 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: - Before, with `-Ctarget-feature=-feat`, if there is some other feature `x` that implies `feat` we would *not* add `-x` to the backend target feature set. Now, we do. This fixes #134792. - The logic that removes `x` from `cfg(target_feature)` in this case also changed a bit, avoiding a large number of calls to the (uncached) `sess.target.implied_target_features` (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before `tcx` so we can't use that... - Previously, if feature "a" implied "b" and "b" was unstable, then using `-Ctarget-feature=+a` would also emit a warning about `b`. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway. The fourth commit increases consistency of the GCC backend with the LLVM backend. The last commit does some further cleanup: - Get rid of RUSTC_SPECIAL_FEATURES. It was only needed for s390x "backchain", but since LLVM 19 that is always a regular target feature so we don't need this hack any more. The hack also has various unintended side-effects so we don't want to keep it. Fixes #142412. - Move RUSTC_SPECIFIC_FEATURES handling into the shared parse_rust_feature_flag helper so all consumers of `-Ctarget-feature` that only care about actual target features (and not "crt-static") have it. Previously, we actually set `cfg(target_feature = "crt-static")` twice: once in the backend target feature logic, and once specifically for that one feature. IIUC, some targets are meant to ignore `-Ctarget-feature=+crt-static`, it seems like before this PR that flag still incorrectly enabled `cfg(target_feature = "crt-static")` (but I didn't test this). - Move fixed_x18 handling together with retpoline handling. - Forbid setting fixed_x18 as a regular target feature, even unstably. It must be set via the `-Z` flag. ``@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
deleted the
target-feature-unification
branch
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request
Jun 28, 2025christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request
Jan 27, 2026christian-schilling pushed a commit to christian-schilling/rustc_codegen_cranelift that referenced this pull request
Jan 27, 2026This 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