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

avx512gfni and avx512vaes feature names might be misleading #1325

Closed
calebzulawski opened this issue Aug 19, 2022 · 6 comments · Fixed by #1349
Closed

avx512gfni and avx512vaes feature names might be misleading #1325

calebzulawski opened this issue Aug 19, 2022 · 6 comments · Fixed by #1349

Comments

@calebzulawski
Copy link
Member

I already opened rust-lang/rust#100752, but this might be the more relevant location. More details are in the attached issue, but there exist CPUs that have GFNI and VAES without AVX-512, and Intel doesn't seem to classify them as part of AVX-512. It might be better to name them simply gfni and vaes.

@SchrodingerZhu
Copy link

so as vpclmulqdq

@Amanieu
Copy link
Member

Amanieu commented Oct 27, 2022

Intel's documentation has these intrinsics marked as requiring AVX512-VL. But if there really is a CPU that has GFNI without AVX512 then you can send a PR similar to #1348 to fix this.

@calebzulawski
Copy link
Member Author

Tremont supports GFNI with smaller vectors only.

@calebzulawski
Copy link
Member Author

calebzulawski commented Oct 30, 2022

Thanks! I think it would still make sense to rename the target features and detection names to not include "avx512", since they do not imply AVX512F. I'm not sure how that would work, though, since it would need to be synchronized between this repository and the main rust repo.

@Amanieu
Copy link
Member

Amanieu commented Oct 30, 2022

I feel that issue should be discussed in rust-lang/rust#100752 since feature names are defined in rustc, not stdarch. Once a decision has been made on that issue then we can make the necessary changes to stdarch. I expect the most likely resolution would be to add feature name aliases, since we need to keep the old names for backward compatibility.

@calebzulawski
Copy link
Member Author

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants