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

Consider renaming the avx512gfni feature to just gfni #100752

Closed
calebzulawski opened this issue Aug 19, 2022 · 5 comments · Fixed by #107707
Closed

Consider renaming the avx512gfni feature to just gfni #100752

calebzulawski opened this issue Aug 19, 2022 · 5 comments · Fixed by #107707
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug.

Comments

@calebzulawski
Copy link
Member

While GFNI has (optional) AVX-512 instructions, there is at least one microarchitecture (Tremont) that has GFNI without AVX-512.

This is further confirmed by running:

rustc +nightly -Ctarget-feature=+avx512gfni --print cfg | grep target_feature
target_feature="avx512gfni"
target_feature="cmpxchg16b"
target_feature="fxsr"
target_feature="llvm14-builtins-abi"
target_feature="sse"
target_feature="sse2"
target_feature="sse3"
target_feature="ssse3"

Unlike other AVX-512 features, avx512f is not enabled, so I'm not sure this should be considered an AVX-512 feature.

@calebzulawski calebzulawski added the C-bug Category: This is a bug. label Aug 19, 2022
@calebzulawski
Copy link
Member Author

The same goes for avx512vaes to vaes, which seems to be implemented on Zen 3 without AVX-512.

@workingjubilee
Copy link
Contributor

This.. seems incorrect. The versions that don't build off and include AVX512 should be separate features, if I remember the ad copy around these.

@calebzulawski
Copy link
Member Author

I'm not sure if you mean my suggestion is incorrect, or the existing feature names, but rustc certainly isn't the first to be confused by the new convention used here: https://community.intel.com/t5/Intel-ISA-Extensions/GFNI-documentation-anomaly/td-p/1153193

@workingjubilee
Copy link
Contributor

I am mostly expressing bafflement at the current situation.

@SchrodingerZhu
Copy link

Please also see rust-lang/stdarch#1343

On AMD platform, VAES (I doubt VPCLMULQDQ) codegen would suffer from the similar issue mentioned here.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue 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 issue 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 issue 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```
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue 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```
@workingjubilee workingjubilee added the A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. label Mar 3, 2023
@bors bors closed this as completed in ce5919f May 15, 2023
antoyo pushed a commit to antoyo/rust that referenced this issue 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
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants