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

Fix some misleading target feature aliases #103750

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Conversation

calebzulawski
Copy link
Member

This is the first half of a fix for #100752. It looks like these aliases were added in #78361 and slipped under the radar, as these features are not AVX512. These features do add AVX512 instructions when used in combination with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions. A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features). This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the std::arch intrinsics to use these names, and finally remove these aliases from rustc.

r? @workingjubilee

cc @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 Oct 30, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2022

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

compiler/rustc_codegen_llvm/src/llvm_util.rs Outdated Show resolved Hide resolved
Comment on lines +318 to +321
adx, aes, avx, avx2, avx512bf16, avx512bitalg, avx512bw, avx512cd, avx512dq, avx512er, avx512f, avx512ifma,
avx512pf, avx512vbmi, avx512vbmi2, avx512vl, avx512vnni, avx512vp2intersect, avx512vpopcntdq,
bmi1, bmi2, cmpxchg16b, ermsb, f16c, fma, fxsr, gfni, lzcnt, movbe, pclmulqdq, popcnt, rdrand, rdseed, rtm,
sha, sse, sse2, sse3, sse4.1, sse4.2, sse4a, ssse3, tbm, vaes, vpclmulqdq, xsave, xsavec, xsaveopt, xsaves
Copy link
Member

Choose a reason for hiding this comment

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

I feel almost like these should be sorted by theme rather than alphabetical, but that can happen some other day.

@@ -179,6 +179,7 @@ const X86_ALLOWED_FEATURES: &[(&str, Option<Symbol>)] = &[
("f16c", Some(sym::f16c_target_feature)),
("fma", None),
("fxsr", None),
("gfni", Some(sym::avx512_target_feature)),
Copy link
Member

Choose a reason for hiding this comment

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

It's honestly kind of weird that this isn't prefixed v for vgfni like the other features.

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 it's because the v prefix implies VEX- or EVEX-encoded instructions (e.g. VGF2P8AFFINEQB), but GFNI also introduces plain SSE instructions (e.g. GF2P8AFFINEQB). This is the same distinction as PCLMULQDQ vs VPCLMULQDQ or AES vs VAES for example, but the difference there is the different encodings have separate CPUID flags, whereas GFNI has just one.

@workingjubilee
Copy link
Member

Looks good to me. @bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2022

📌 Commit 2a7c3d02722704e96095a9d734d9db3bbe72ab56 has been approved by workingjubilee

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2022
calebzulawski and others added 2 commits November 12, 2022 18:46
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
@calebzulawski
Copy link
Member Author

I accidentally synced my fork, didn't realize this PR was linked to my master branch. Not sure if you'll need to reapprove the rebased commits...

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2022

📌 Commit 1122400 has been approved by workingjubilee

It is now in the queue for this repository.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 16, 2022
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? `@workingjubilee`

cc `@Amanieu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 16, 2022
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ``@workingjubilee``

cc ``@Amanieu``
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2022
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#103750 (Fix some misleading target feature aliases)
 - rust-lang#104137 (Issue error when -C link-self-contained option is used on unsupported platforms)
 - rust-lang#104317 (cleanup and dedupe CTFE and Miri error reporting)
 - rust-lang#104335 (Only do parser recovery on retried macro matching)
 - rust-lang#104394 (various cleanups to try to reduce the use of spans inside method resolution)
 - rust-lang#104459 (rustdoc: remove unused JS IIFE from main.js)
 - rust-lang#104462 (rustdoc: remove pointless CSS `.rightside { padding-right: 2px }`)
 - rust-lang#104466 (rustdoc: remove no-op CSS `#crate-search-div { display: inline-block }`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 56a28a6 into rust-lang:master Nov 16, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 16, 2022
@Amanieu Amanieu mentioned this pull request Dec 16, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ```@workingjubilee```

cc ```@Amanieu```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#103750 (Fix some misleading target feature aliases)
 - rust-lang#104137 (Issue error when -C link-self-contained option is used on unsupported platforms)
 - rust-lang#104317 (cleanup and dedupe CTFE and Miri error reporting)
 - rust-lang#104335 (Only do parser recovery on retried macro matching)
 - rust-lang#104394 (various cleanups to try to reduce the use of spans inside method resolution)
 - rust-lang#104459 (rustdoc: remove unused JS IIFE from main.js)
 - rust-lang#104462 (rustdoc: remove pointless CSS `.rightside { padding-right: 2px }`)
 - rust-lang#104466 (rustdoc: remove no-op CSS `#crate-search-div { display: inline-block }`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2023
…nieu

Remove misleading target feature aliases

Fixes rust-lang#100752.  This is a follow up to rust-lang#103750. These aliases could not be completely removed until rust-lang/stdarch#1355 landed.

cc `@Amanieu`
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
…ilee

Fix some misleading target feature aliases

This is the first half of a fix for rust-lang#100752.  It looks like these aliases were added in rust-lang#78361 and slipped under the radar, as these features are not AVX512.  These features _do_ add AVX512 instructions when used _in combination_ with AVX512F, but without AVX512F, these features still provide 128-bit and 256-bit vector instructions.  A user might be mislead into thinking these features imply AVX512F (which is true of the actual AVX512 features).  This PR allows using the names as defined by LLVM, which matches Intel documentation.

A future PR should change the `std::arch` intrinsics to use these names, and finally remove these aliases from rustc.

r? ```@workingjubilee```

cc ```@Amanieu```
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
Development

Successfully merging this pull request may close these issues.

4 participants