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.

Sign up for GitHub

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

Jump to bottom

-Ctarget-feature=+avx doesn't enable sse4.2 #128426

Closed
calebzulawski opened this issue Jul 31, 2024 · 8 comments · Fixed by #128221
Closed

-Ctarget-feature=+avx doesn't enable sse4.2 #128426

calebzulawski opened this issue Jul 31, 2024 · 8 comments · Fixed by #128221
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

@calebzulawski
Copy link
Member

See example: https://rust.godbolt.org/z/GxMaecr4T

Adding either sse4.2 or crc32 works, so I believe the issue is due to the tied crc32 feature.

This issue is present on latest master.
@calebzulawski calebzulawski added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. labels Jul 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 31, 2024
calebzulawski added a commit to calebzulawski/rust that referenced this issue Jul 31, 2024
@calebzulawski
Add +sse4.2 due to rust-lang#128426
02f6b8e
calebzulawski added a commit to calebzulawski/rust that referenced this issue Jul 31, 2024
@calebzulawski
Add +sse4.2 due to rust-lang#128426
b3dcd5d
@calebzulawski calebzulawski mentioned this issue Jul 31, 2024
Merged
@RalfJung
Copy link
Member

Seems like LLVM doesn't consider this to be an implication. I don't know enough about target features to be able to say who is right here.

Cc @nikic

@calebzulawski
Copy link
Member Author

X86.td does define avx as implying sse4.2 and -mavx in clang enables sse4.2, I think we're getting it lost somewhere because we define sse4.2 as enabling both sse4.2 and crc32. sse4.2 is the only x86 SIMD feature tied to another feature.

@RalfJung
Copy link
Member

RalfJung commented Jul 31, 2024 •
edited
Loading

Ah, I see. So I guess we must have special logic then that determines when cfg!(target_feature = "sse4.2") holds (checking that both sse4.2 and crc32 are enabled), and maybe that logic is wrong and doesn't properly look at the LLVM-expanded version of the attribute set.

@RalfJung
Copy link
Member

-mavx in clang enables sse4.2,

Does it enable crc32 as well?

@calebzulawski
Copy link
Member Author

Yes, it does: https://c.godbolt.org/z/rrd7Pvr4T

@calebzulawski
Copy link
Member Author

I'd be curious to see if this is a problem with other tied features, not just sse4.2

calebzulawski added a commit to calebzulawski/rust that referenced this issue Aug 3, 2024
@calebzulawski
Revert "Add +sse4.2 due to rust-lang#128426"
7784dbd 
This reverts commit b3dcd5d.
@calebzulawski
Copy link
Member Author

calebzulawski commented Aug 3, 2024 •
edited
Loading

Looking into it a bit more, I think it's incorrect for sse4.2 to enable crc32: https://reviews.llvm.org/D105462

crc32 appears to be a special feature that enables the crc32 instruction when other vector extensions are disabled, so I don't think it's ever necessary when sse4.2 is enabled. I think clang enables the __CRC32__ attribute if one of sse4.2 or crc32 are enabled.

@calebzulawski
Copy link
Member Author

This also has the consequence of causing sse4.2 to not inline into avx: https://rust.godbolt.org/z/KMGnffTnY

calebzulawski added a commit to calebzulawski/rust that referenced this issue Aug 4, 2024
@calebzulawski
Add +sse4.2 due to rust-lang#128426
81a0a47
calebzulawski added a commit to calebzulawski/rust that referenced this issue Aug 4, 2024
@calebzulawski
Revert "Add +sse4.2 due to rust-lang#128426"
2f890e7 
This reverts commit b3dcd5d.
calebzulawski added a commit to calebzulawski/rust that referenced this issue Aug 6, 2024
@calebzulawski
Add +sse4.2 due to rust-lang#128426
a3ffd8d
tgross35 added a commit to tgross35/rust that referenced this issue Aug 7, 2024
@tgross35
Rollup merge of rust-lang#128221 - calebzulawski:implied-target-featu…
8b65b64 
…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 issue Aug 7, 2024
@tgross35
Rollup merge of rust-lang#128221 - calebzulawski:implied-target-featu…
9c09e87 
…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``
calebzulawski added a commit to calebzulawski/rust that referenced this issue Aug 7, 2024
@calebzulawski
Add +sse4.2 due to rust-lang#128426
34f29a2
tgross35 added a commit to tgross35/rust that referenced this issue Aug 7, 2024
@tgross35
Rollup merge of rust-lang#128221 - calebzulawski:implied-target-featu…
0665f3c 
…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 issue Aug 7, 2024
@tgross35
Rollup merge of rust-lang#128221 - calebzulawski:implied-target-featu…
044456d 
…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 bors closed this as completed in 904f579 Aug 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 7, 2024
@rust-timer
Unrolled build for rust-lang#128221
de1e144 
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``
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add implied target features to target_feature attribute calebzulawski/rust
4 participants
@RalfJung @calebzulawski @saethlin @rustbot