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 zen3 flags for icc and support icelake #321

Merged
merged 3 commits into from
Mar 24, 2022

Conversation

bernhardmgruber
Copy link
Collaborator

@bernhardmgruber bernhardmgruber commented Mar 23, 2022

This PR fixes the flags passed to icc, when a zen3 CPU was detected. It furthermore let's icc build the AVX2 Vc static library without Intel specific instruction sets. To make the CI pass, also support for icelake CPUs had to be cherry-picked from #319.

@bernhardmgruber
Copy link
Collaborator Author

This might not work yet, because the Vc static library for AVX2 compiles with -xCORE-AVX2 by default :/
See: https://github.com/VcDevel/Vc/blob/1.4/cmake/VcMacros.cmake#L493-L500

@bernhardmgruber bernhardmgruber changed the title Fix zen3 flags for icc Fix zen3 flags for icc and support icelake Mar 23, 2022
@bernhardmgruber bernhardmgruber mentioned this pull request Mar 23, 2022
@bernhardmgruber bernhardmgruber force-pushed the zen3fix branch 2 times, most recently from 15e8240 to 690481f Compare March 23, 2022 15:59
@@ -495,7 +495,7 @@ macro(vc_compile_for_all_implementations _srcs _src)
# which case AVX2 implies the availability of FMA and BMI2
#_vc_compile_one_implementation(${_srcs} AVX2 "-mavx2")
#_vc_compile_one_implementation(${_srcs} AVX2+BMI2 "-mavx2 -mbmi2")
_vc_compile_one_implementation(${_srcs} AVX2+FMA+BMI2 "-xCORE-AVX2" "-mavx2 -mfma -mbmi2" "/arch:AVX2")
_vc_compile_one_implementation(${_srcs} AVX2+FMA+BMI2 "-xAVX2" "-mavx2 -mfma -mbmi2" "/arch:AVX2")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used on AMD?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. And funnily it works. Because the unit tests did not work with -xAVX2, I needed -mavx2. Should we change to -mavx2 here as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that anything that could be run on AMD should fallback to -mavx2 so that the compiler doesn't add special code based on cpuid checks (for example, if may just fallback to scalar if the AMD CPU is not recognized, or even intentionally).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will send a separate PR for that.

@amadio amadio merged commit 61efa28 into VcDevel:1.4 Mar 24, 2022
@bernhardmgruber bernhardmgruber deleted the zen3fix branch March 24, 2022 13:37
@bernhardmgruber bernhardmgruber added this to the Vc 1.4.3 milestone Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants