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

Target feature implications for negative features are handled inconsistently between codegen and cfg(target_feature) #134792

Open
RalfJung opened this issue Dec 26, 2024 · 1 comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Dec 26, 2024

The logic that computes cfg(target_feature) takes into account target feature implications when handling something like -sse: it will also remove avx from the list of enabled target features in that case. However, the logic that computes which flags we set for codegen (which, unfortunately, is completely separate), does not do the same: it will add -sse to LLVM's target feature list, but does not do anything about Rust-level target feature implications.

This can't be correct -- either negative target features also imply that their "reverse dependencies" get disabled, or they do not. We shouldn't do one thing in codegen and a different thing for cfg. Or is there some good reason for this?

The logic for this was added in #128221.

Cc @calebzulawski @Amanieu @workingjubilee

@RalfJung RalfJung added the C-bug Category: This is a bug. label Dec 26, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 26, 2024
@workingjubilee
Copy link
Member

If I understand this issue description correctly (95% sure, will have to review the actual code to be ~99.9%) then no, there is no particularly good reason for this difference and we should do the exact same thing.

@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants