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.

Sign up for GitHub

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

Jump to bottom

Add a64fx and fujitsu Compiler support #3614

Merged
merged 3 commits into from
Nov 24, 2020
Merged

Add a64fx and fujitsu Compiler support #3614

merged 3 commits into from
Nov 24, 2020

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Nov 21, 2020

This adds support for A64FX (ARM v8.2 with SVE) and the Fujitsu Compiler in order to support Fugaku.

@crtrott
Add A64FX (ARMv8 with SVE) support
ff8a2e7
@crtrott
Add support for Fujitsu compiler
174534c 
Fujitsu compiler is not properly recognized by Cmake, so we need
to work around that
dalg24
dalg24 reviewed Nov 22, 2020
View reviewed changes
cmake/kokkos_compiler_id.cmake Outdated
@@ -137,6 +137,17 @@ IF(KOKKOS_CXX_COMPILER_ID STREQUAL Cray OR KOKKOS_CLANG_IS_CRAY)
ENDIF()
ENDIF()

IF(KOKKOS_CXX_COMPILER_ID STREQUAL Fujitsu)
# SET Cray's compiler version.
Copy link
Member

Choose a reason for hiding this comment

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

I assume you copied from above and forgot to update or remove the comment

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah ...

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

core/unit_test/CMakeLists.txt
@@ -18,7 +18,7 @@ TARGET_COMPILE_DEFINITIONS(kokkos_gtest PUBLIC GTEST_HAS_TR1_TUPLE=0 GTEST_HAS_P

TARGET_INCLUDE_DIRECTORIES(kokkos_gtest PUBLIC ${GTEST_SOURCE_DIR})
#Gtest minimally requires C++11
IF(NOT (Kokkos_ENABLE_CUDA AND WIN32))
IF((NOT (Kokkos_ENABLE_CUDA AND WIN32)) AND (NOT ("${KOKKOS_CXX_COMPILER_ID}" STREQUAL "Fujitsu")))
TARGET_COMPILE_FEATURES(kokkos_gtest PUBLIC cxx_std_11)
Copy link
Member

Choose a reason for hiding this comment

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

Would someone remind me why we need this in the first place? Don't we set CMAKE_CXX_STANDARD?

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 thought someone found this was needed on some platform ...

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, we don't compile gtest with the Kokkos flags and we don't require CMAKE_CXX_STANDARD directly. Hence, there were some issues in the nightlies at some point.

Copy link
Member

Choose a reason for hiding this comment

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

I would be surprised if we still need that line at all but this probably shouldn't be addressed here

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 was suprised we had issues to begin with. And yes somewhere we needed that with some combination of weird compiler (Intel XL or so) and some cmake version.

cmake/kokkos_arch.cmake Outdated
@@ -35,6 +35,7 @@ KOKKOS_ARCH_OPTION(ARMV80 HOST "ARMv8.0 Compatible CPU")
KOKKOS_ARCH_OPTION(ARMV81 HOST "ARMv8.1 Compatible CPU")
KOKKOS_ARCH_OPTION(ARMV8_THUNDERX HOST "ARMv8 Cavium ThunderX CPU")
KOKKOS_ARCH_OPTION(ARMV8_THUNDERX2 HOST "ARMv8 Cavium ThunderX2 CPU")
KOKKOS_ARCH_OPTION(A64FX HOST "ARMv8.2 with SVE Suport")
Copy link
Member

Choose a reason for hiding this comment

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

Do not break the alignment.

Copy link
Member Author

Choose a reason for hiding this comment

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

bah that came through the renaming from ARMV8_SVE to A64FX based on feedback from @nmhamster . Gonna fix that,

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@crtrott
Fix warning with Fujitsu and some formatting fixes
f2e6c63 
The fujitsu compile actually does warn about that comment,
thinking we are using an unsupported tri-graph ...
@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Nov 23, 2020
masterleinad
masterleinad approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me (assuming that the flags are actually correct).

dalg24
dalg24 reviewed Nov 23, 2020
View reviewed changes
core/src/impl/Kokkos_TaskQueue_impl.hpp Show resolved Hide resolved
core/unit_test/CMakeLists.txt
@@ -18,7 +18,7 @@ TARGET_COMPILE_DEFINITIONS(kokkos_gtest PUBLIC GTEST_HAS_TR1_TUPLE=0 GTEST_HAS_P

TARGET_INCLUDE_DIRECTORIES(kokkos_gtest PUBLIC ${GTEST_SOURCE_DIR})
#Gtest minimally requires C++11
IF(NOT (Kokkos_ENABLE_CUDA AND WIN32))
IF((NOT (Kokkos_ENABLE_CUDA AND WIN32)) AND (NOT ("${KOKKOS_CXX_COMPILER_ID}" STREQUAL "Fujitsu")))
TARGET_COMPILE_FEATURES(kokkos_gtest PUBLIC cxx_std_11)
Copy link
Member

Choose a reason for hiding this comment

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

I would be surprised if we still need that line at all but this probably shouldn't be addressed here

cmake/kokkos_compiler_id.cmake
Comment on lines +140 to +149
IF(KOKKOS_CXX_COMPILER_ID STREQUAL Fujitsu)
# SET Fujitsus compiler version which is not detected by CMake
EXECUTE_PROCESS(COMMAND ${CMAKE_CXX_COMPILER} --version
OUTPUT_VARIABLE INTERNAL_CXX_COMPILER_VERSION
OUTPUT_STRIP_TRAILING_WHITESPACE)

STRING(REGEX MATCH "[0-9]+\\.[0-9]+\\.[0-9]+"
TEMP_CXX_COMPILER_VERSION ${INTERNAL_CXX_COMPILER_VERSION})
SET(KOKKOS_CXX_COMPILER_VERSION ${TEMP_CXX_COMPILER_VERSION} CACHE STRING INTERNAL FORCE)
ENDIF()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we actually need this block. Why did you add it?

Copy link
Member Author

Choose a reason for hiding this comment

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

because otherwise KOKKOS_CXX_COMPILER_VERSION is empty and some code later fails.

dhollman
dhollman reviewed Nov 23, 2020
View reviewed changes
algorithms/unit_tests/CMakeLists.txt
@@ -24,7 +24,7 @@ KOKKOS_ADD_TEST_LIBRARY(
# avoid deprecation warnings from MSVC
TARGET_COMPILE_DEFINITIONS(kokkosalgorithms_gtest PUBLIC GTEST_HAS_TR1_TUPLE=0 GTEST_HAS_PTHREAD=0)

IF(NOT (Kokkos_ENABLE_CUDA AND WIN32))
IF((NOT (Kokkos_ENABLE_CUDA AND WIN32)) AND (NOT ("${KOKKOS_CXX_COMPILER_ID}" STREQUAL "Fujitsu")))
Copy link

Choose a reason for hiding this comment

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

Elaborate on why this exception is here in a comment, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

because cmake is weird and I need to do this to make it work?

Copy link

Choose a reason for hiding this comment

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

This can't be just a CMake problem. @jjwilke or is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

it kinda is, because it combines requesting a c++ standard via compile feature (instead of just setting a cxx flag) with checking whether the compiler actually supports it, which it comes back as negative if it doesn't identify the compiler as a known thing (like nvcc on windows, or fujitsu in gener). I guess I can add that as a comment.

Makefile.kokkos Show resolved Hide resolved
cmake/kokkos_test_cxx_std.cmake Show resolved Hide resolved
cmake/kokkos_test_cxx_std.cmake Show resolved Hide resolved
dhollman
dhollman approved these changes Nov 24, 2020
View reviewed changes
Copy link

@dhollman dhollman left a comment

Choose a reason for hiding this comment

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

It's a mess, but all of our build system is a mess apparently. In that context, LGTM.

@crtrott
Copy link
Member Author

crtrott commented Nov 24, 2020

Opened #3620 which is there to address some of the questions here medium term and streamline the process of side stepping cmake mines for unknown compilers.

@crtrott crtrott merged commit bb176c8 into kokkos:develop Nov 24, 2020
@crtrott crtrott deleted the add-a64fx-and-fujitsu branch November 24, 2020 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhollman dhollman dhollman approved these changes

@dalg24 dalg24 dalg24 approved these changes

@masterleinad masterleinad masterleinad approved these changes

Assignees
No one assigned
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@crtrott @dhollman @dalg24 @masterleinad