-
Notifications
You must be signed in to change notification settings - Fork 407
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
Add icelake as CPU architecture #4929
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please confirm that KOKKOS_ENABLE_TM
does not need to be set
Wikipedia says (https://en.wikipedia.org/wiki/Transactional_Synchronization_Extensions):
Hence, it seems that at least some Ice Lake CPUs don't support transactional memory. |
Co-authored-by: Damien L-G <dalg24@gmail.com>
…TERNAL_USE_ARCH_AVX512XEON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change the AVX512? Note that there is unfortunately a difference between server grade SKX (which our setting uses) and non-server. The same for icelake. We probably need to distinguish.
@@ -426,10 +427,9 @@ KOKKOS_INTERNAL_USE_ARCH_SSE42 := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_W | |||
KOKKOS_INTERNAL_USE_ARCH_AVX := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_SNB) + $(KOKKOS_INTERNAL_USE_ARCH_AMDAVX)) | |||
KOKKOS_INTERNAL_USE_ARCH_AVX2 := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_HSW) + $(KOKKOS_INTERNAL_USE_ARCH_BDW) + $(KOKKOS_INTERNAL_USE_ARCH_ZEN) + $(KOKKOS_INTERNAL_USE_ARCH_ZEN2) + $(KOKKOS_INTERNAL_USE_ARCH_ZEN3)) | |||
KOKKOS_INTERNAL_USE_ARCH_AVX512MIC := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_KNL)) | |||
KOKKOS_INTERNAL_USE_ARCH_AVX512XEON := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_SKX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer
KOKKOS_INTERNAL_USE_ARCH_AVX512XEON := $(shell expr $(KOKKOS_INTERNAL_USE_ARCH_SKX) + $(KOKKOS_INTERNAL_USE_ARCH_ICX))
and then later
ifeq ($(KOKKOS_INTERNAL_USE_ARCH_AVX512XEON), 1)
tmp := $(call kokkos_append_header,"$H""define KOKKOS_ARCH_AVX512XEON")
endif
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering about making SKX the SKX512 or SKX_SERVER
and ICX ICX512 or ICX_SERVER
The difference of the server variant is that it got AVX512
the icelake-server gcc option probably reflects that.
Then we could make SKX and ICX the none-server variant, which is I think how spack handles it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Makefile.kokkos
KOKKOS_INTERNAL_USE_ARCH_AVX512XEON
is only used to set flags for Skylake, though,
KOKKOS_CXXFLAGS += -march=skylake-avx512 -mtune=skylake-avx512 -mrtm
Enabling it for Ice Lake wouldn't allow us to set the desired architecture flags. Hence, I decided to get rid of KOKKOS_INTERNAL_USE_ARCH_AVX512XEON
altogether and just set the preprocessor variable KOKKOS_ARCH_AVX512XEON
directly according to the architecture.
The alternative to this is what Damien is proposing.
The instruction set supported for Ice Lake is more similar to Skylake than to Knights Landing so it seemed appropriate to use The flags we are using for Skylake are the server ones ( |
https://spack.readthedocs.io/en/latest/basic_usage.html#support-for-specific-microarchitectures shows that
as microarchitecture. Also, I don't know if we need to be concerned with backward compatibility. According to https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(client) and https://en.wikichip.org/wiki/intel/microarchitectures/ice_lake_(server) both Ice Lake variants support AVX512. |
Folks - SKX means Sky Lake Xeon (which is the server), SKL (Sky Lake) is the client part. It might be good to try to use this naming so we have similar terms to those that Intel uses. ICX and ICL follow similar patterns. |
OK I like Si's comment here. |
@crtrott I think in that case this pull request does the right thing (adding IceLake server as ICX to both the CMake build system and Makefile.kokkos; SKX continues to mean Skylake server). |
@masterleinad can you resolve the conflict? We also should probably add SKL and ICL as Si mentioned but could do that in a different one. |
Done. I'll add |
Retest this please |
So far I didn't touch the
Makefile
because I don't quite understand what kind of variables I should set for icelake. InCMake
we just try-march=icelake-server -mtune=icelake-server
and don't set anything else.