Add implied target features to target_feature attribute #128221

calebzulawski · 2024-07-26T04:18:36Z

See zulip for some context. Adds implied target features, e.g. #[target_feature(enable = "avx2")] acts like #[target_feature(enable = "avx2,avx,sse4.2,sse4.1...")]. Fixes #128125, fixes #128426

The implied feature sets are taken from the rust reference, there are certainly more features and targets to add.

Please feel free to reassign this to whoever should review it.

r? @Amanieu

Amanieu · 2024-07-26T10:37:55Z

Can you also make this work with asm_target_features? This would allow you to use ymm registers by enabling avx512f (which currently only enables zmm registers).

calebzulawski · 2024-07-26T14:06:24Z

As far as I can tell, this change fixes that as well. I added the test from the linked issue and fixed the other failing tests, too

calebzulawski · 2024-07-26T15:30:15Z

I'm not thrilled with the changes necessary to tests/codegen/target-feature-overrides.rs, I needed to add additional -Ctarget-feature features to get it to inline. Strange, since LLVM should know those features are implied and should allow inlining.

daxpedda · 2024-07-27T16:27:28Z

I'm not thrilled with the changes necessary to tests/codegen/target-feature-overrides.rs, I needed to add additional -Ctarget-feature features to get it to inline. Strange, since LLVM should know those features are implied and should allow inlining.

I had to add a much simpler version of this in #117468.
So if #117468 has to be rebased I can just use this implementation, if this PR has to be rebased then just overwrite #117468's implementation.

In #117468's case it should be fixed in LLVM v19 I believe, but maybe it would encounter the same problem with inlining as well.

calebzulawski · 2024-07-29T00:43:50Z

I'm not thrilled with the changes necessary to tests/codegen/target-feature-overrides.rs, I needed to add additional -Ctarget-feature features to get it to inline. Strange, since LLVM should know those features are implied and should allow inlining.

I had to add a much simpler version of this in #117468. So if #117468 has to be rebased I can just use this implementation, if this PR has to be rebased then just overwrite #117468's implementation.

I'm not sure about the merge order, but I can add the relaxed-simd dependency to this PR :)

daxpedda · 2024-07-29T05:58:14Z

I'm not thrilled with the changes necessary to tests/codegen/target-feature-overrides.rs, I needed to add additional -Ctarget-feature features to get it to inline. Strange, since LLVM should know those features are implied and should allow inlining.

I had to add a much simpler version of this in #117468. So if #117468 has to be rebased I can just use this implementation, if this PR has to be rebased then just overwrite #117468's implementation.

I'm not sure about the merge order, but I can add the relaxed-simd dependency to this PR :)

Thanks!

rustbot · 2024-07-31T03:59:28Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-07-31T05:47:51Z

compiler/rustc_target/src/target_features.rs

@@ -407,6 +407,79 @@ const IBMZ_ALLOWED_FEATURES: &[(&str, Stability)] = &[
    // tidy-alphabetical-end
 ];

+const X86_IMPLIED_FEATURES: &[(&str, &[&str])] = &[


Is it necessary to have this list here?

So far, we handle implied target features by having LLVM compute the full list of target features. That way, -C target-feature=avx2 makes cfg!(target_feature="avx") be true -- at least that is my understanding.

We shouldn't have two different implied-target-feature mechanisms: either everything should use LLVM, or everything should use this table.

There seemed to be some desire in zulip to keep it separate, but I can do that (if I can find where cfg is set from LLVM...)

Where on Zulip is that desire expressed? All I can see i Amanieu saying we should have our own implication table, but I don't see anyone even talking about the concern that different parts of rustc would have different user-visible notions of "implied target features".

I think that we should handle implied target features entirely on the rustc side and only use LLVM to query target features when -C target-cpu is used (at least until we have our own mechanism for that). I want to move away from LLVM's target feature and into something that is backend-agnostic.

RalfJung · 2024-07-31T05:56:51Z

It would seem really strange to me if cfg! and "whether inline asm can use a register" have a different view of whether target feature is enabled.

RalfJung · 2024-07-31T06:05:55Z

Given that this makes more code compile on stable, we probably should involve t-lang here before landing anything.

calebzulawski · 2024-07-31T06:06:43Z

It would seem really strange to me if cfg! and "whether inline asm can use a register" have a different view of whether target feature is enabled.

In retrospect, I think this is due to #128426 as well. I don't think this is even necessarily fixed by switching the tables to LLVM, since LLVM definitely enables sse4.2 for avx

rustbot · 2024-08-02T04:21:04Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rustbot · 2024-08-05T03:52:09Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Co-authored-by: Amanieu d'Antras <[email protected]>

calebzulawski · 2024-08-07T06:19:17Z

@bors r=Amanieu

bors · 2024-08-07T06:19:19Z

📌 Commit 8818c95 has been approved by Amanieu

It is now in the queue for this repository.

…res, r=Amanieu Add implied target features to target_feature attribute See [zulip](https://rust-lang.zulipchat.com/#narrow/stream/208962-t-libs.2Fstdarch/topic/Why.20would.20target-feature.20include.20implied.20features.3F) for some context. Adds implied target features, e.g. `#[target_feature(enable = "avx2")]` acts like `#[target_feature(enable = "avx2,avx,sse4.2,sse4.1...")]`. Fixes rust-lang#128125, fixes rust-lang#128426 The implied feature sets are taken from [the rust reference](https://doc.rust-lang.org/reference/attributes/codegen.html?highlight=target-fea#x86-or-x86_64), there are certainly more features and targets to add. Please feel free to reassign this to whoever should review it. r? `@Amanieu`

Rollup of 7 pull requests Successful merges: - rust-lang#128206 (Make create_dll_import_lib easier to implement) - rust-lang#128221 (Add implied target features to target_feature attribute) - rust-lang#128384 (Add tests to ensure MTE tags are preserved across FFI boundaries) - rust-lang#128656 (Enable msvc for run-make/rust-lld) - rust-lang#128691 (Update `compiler-builtins` to 0.1.117) - rust-lang#128700 (Migrate `simd-ffi` `run-make` test to rmake) - rust-lang#128758 (Specify a minimum supported version for VxWorks) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128221 (Add implied target features to target_feature attribute) - rust-lang#128261 (impl `Default` for collection iterators that don't already have it) - rust-lang#128353 (Change generate-copyright to generate HTML, with cargo dependencies included) - rust-lang#128679 (codegen: better centralize function declaration attribute computation) - rust-lang#128732 (make `import.vis` is immutable) - rust-lang#128755 (Integrate crlf directly into related test file instead via of .gitattributes) - rust-lang#128772 (rustc_codegen_ssa: Set architecture for object crate for 32-bit SPARC) - rust-lang#128782 (unused_parens: do not lint against parens around &raw) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128221 - calebzulawski:implied-target-features, r=Amanieu Add implied target features to target_feature attribute See [zulip](https://rust-lang.zulipchat.com/#narrow/stream/208962-t-libs.2Fstdarch/topic/Why.20would.20target-feature.20include.20implied.20features.3F) for some context. Adds implied target features, e.g. `#[target_feature(enable = "avx2")]` acts like `#[target_feature(enable = "avx2,avx,sse4.2,sse4.1...")]`. Fixes rust-lang#128125, fixes rust-lang#128426 The implied feature sets are taken from [the rust reference](https://doc.rust-lang.org/reference/attributes/codegen.html?highlight=target-fea#x86-or-x86_64), there are certainly more features and targets to add. Please feel free to reassign this to whoever should review it. r? ``@Amanieu``

RalfJung · 2024-09-02T09:58:45Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

-                }
-            }
-            true
+            RUSTC_SPECIAL_FEATURES.contains(feature) || features.contains(&Symbol::intern(feature))


This line makes it so that the RUSTC_SPECIAL_FEATURES are always enabled in cfg, i.e. cfg!(target_feature = "backchain") will always be true. Is that the intended behavior? It seems surprising, and there's no comment explaining the surprise.

It seems like that was also already the behavior before this PR. But it still seems odd...

I think I originally omitted this and tests failed, I didn't look into why this is done

I think it's a mistake originating in #127506.

I filed an issue for this: #129927

RalfJung · 2024-09-03T07:13:58Z

compiler/rustc_codegen_llvm/src/llvm_util.rs

@@ -308,30 +277,61 @@ pub fn check_tied_features(
 /// Used to generate cfg variables and apply features
 /// Must express features in the way Rust understands them
 pub fn target_features(sess: &Session, allow_unstable: bool) -> Vec<Symbol> {


There is a very similar function in compiler/rustc_codegen_gcc/src/lib.rs which now works quite differently from its LLVM counterpart...
Cc @rust-lang/wg-gcc-backend

Thanks. I plan at some point to try to remove duplications between the 2 codegens.

Enabling a tied feature should not enable the other feature automatically. This was fixed by something in rust-lang#128796, probably rust-lang#128221 or rust-lang#128679.

…n, r=wesleywiser codegen_ssa: consolidate tied target checks Fixes rust-lang#105110. Fixes rust-lang#105111. `rustc_codegen_llvm` and `rustc_codegen_gcc` duplicated logic for checking if tied target features were partially enabled. This PR consolidates these checks into `rustc_codegen_ssa` in the `codegen_fn_attrs` query, which also is run pre-monomorphisation for each function, which ensures that this check is run for unused functions, as would be expected. Also adds a test confirming that enabling one tied feature doesn't imply another - the appropriate error for this was already being emitted. I did a bisect and narrowed it down to two patches it was likely to be - something in rust-lang#128796, probably rust-lang#128221 or rust-lang#128679.

Rollup merge of rust-lang#130308 - davidtwco:tied-target-consolidation, r=wesleywiser codegen_ssa: consolidate tied target checks Fixes rust-lang#105110. Fixes rust-lang#105111. `rustc_codegen_llvm` and `rustc_codegen_gcc` duplicated logic for checking if tied target features were partially enabled. This PR consolidates these checks into `rustc_codegen_ssa` in the `codegen_fn_attrs` query, which also is run pre-monomorphisation for each function, which ensures that this check is run for unused functions, as would be expected. Also adds a test confirming that enabling one tied feature doesn't imply another - the appropriate error for this was already being emitted. I did a bisect and narrowed it down to two patches it was likely to be - something in rust-lang#128796, probably rust-lang#128221 or rust-lang#128679.

rustbot assigned Amanieu Jul 26, 2024

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 Jul 26, 2024