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

Add implied target features to target_feature attribute #128221

Merged
merged 14 commits into from
Aug 7, 2024

Conversation

calebzulawski
Copy link
Member

@calebzulawski calebzulawski commented Jul 26, 2024

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

@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 Jul 26, 2024
@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Jul 26, 2024

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
Copy link
Member Author

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

@rust-log-analyzer

This comment has been minimized.

@calebzulawski
Copy link
Member Author

calebzulawski commented Jul 26, 2024

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.

@rust-log-analyzer

This comment has been minimized.

@daxpedda
Copy link
Contributor

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
Copy link
Member Author

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 :)

@rust-log-analyzer

This comment has been minimized.

@daxpedda
Copy link
Contributor

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
Copy link
Collaborator

rustbot commented Jul 31, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

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

const X86_IMPLIED_FEATURES: &[(&str, &[&str])] = &[
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...)

Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

@calebzulawski
Copy link
Member Author

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

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@calebzulawski calebzulawski force-pushed the implied-target-features branch 2 times, most recently from f0c7d4b to fe10ed8 Compare August 4, 2024 06:09
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@calebzulawski
Copy link
Member Author

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Aug 7, 2024

📌 Commit 8818c95 has been approved by Amanieu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2024
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
…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`
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 7, 2024
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
…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
@bors bors merged commit 904f579 into rust-lang:master Aug 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2024
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``
}
}
true
RUSTC_SPECIAL_FEATURES.contains(feature) || features.contains(&Symbol::intern(feature))
Copy link
Member

@RalfJung RalfJung Sep 2, 2024

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I filed an issue for this: #129927

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

davidtwco added a commit to davidtwco/rust that referenced this pull request Sep 13, 2024
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.
davidtwco added a commit to davidtwco/rust that referenced this pull request Sep 13, 2024
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.
davidtwco added a commit to davidtwco/rust that referenced this pull request Sep 24, 2024
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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…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.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Oct 10, 2024
…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.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 10, 2024
…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.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
9 participants