Enable HIP with Cray Clang #3842

dalg24 · 2021-03-11T16:40:05Z

They are a few issues we need to discuss:

~~Duplicated logic to detect Cray Clang in kokkos_compiler_id.cmake~~ now detecting hipcc and not distinguishing Cray Clang from vanilla LLVM
~~https://github.com/dalg24/kokkos/blob/e0f332456a93a89e9c9a2105cf9b0719f0bf9118/cmake/kokkos_compiler_id.cmake#L91-L99~~
~~Minimum versions requirements~~
~~https://github.com/dalg24/kokkos/blob/e0f332456a93a89e9c9a2105cf9b0719f0bf9118/cmake/kokkos_compiler_id.cmake#L163-L166~~
~~HIP offload target architecture flag~~
~~Should we be using --offload-arch=...?~~
~~Logic to get ROCM_PATH~~ relying on find_package(hip) to get it
https://github.com/dalg24/kokkos/blob/e0f332456a93a89e9c9a2105cf9b0719f0bf9118/cmake/kokkos_arch.cmake#L117-L119

cmake/kokkos_arch.cmake

dalg24 · 2021-03-12T00:11:04Z

Note that I configured with

        "CMAKE_EXE_LINKER_FLAGS": "-lamdhip64 -lhsa-runtime64 -L$env{ROCM_PATH}/lib",
        "CMAKE_SHARED_LINKER_FLAGS": "-lamdhip64 -lhsa-runtime64 -L$env{ROCM_PATH}/lib",

so we probably should find the library and handle this from CMake.

…ile HIP

jrmadsen

The main change I would like to see is w.r.t. --rocm-path=$ENV{ROCM_PATH}. We should at least check that is defined in the env. Optionally, we could also allow the user to specify it on the command-line and fallback to the env variable (if that would work with the toolchain).

jrmadsen · 2021-03-13T00:47:13Z

cmake/kokkos_arch.cmake

+  ELSE()
+    SET(AMDGPU_ARCH_FLAG "--offload-arch")
+    GLOBAL_APPEND(KOKKOS_AMDGPU_OPTIONS -x hip)
+    GLOBAL_APPEND(KOKKOS_AMDGPU_OPTIONS --rocm-path=$ENV{ROCM_PATH})


I don't really like relying on environment variables this explicitly. This would cause weird compiler issues if this variable isn't set.

Agreed with @jrmadsen. Referencing the comment below, I think it makes sense to have a FindROCM.cmake file that resolves all these paths. ROCM_PATH would then be a variable.

jrmadsen · 2021-03-13T00:48:44Z

cmake/kokkos_arch.cmake

+    FIND_LIBRARY(LIB_AMD_HIP amdhip64 PATHS ENV ROCM_PATH PATH_SUFFIXES lib)
+    FIND_LIBRARY(LIB_HSA_RUNTIME hsa-runtime64 PATHS ENV ROCM_PATH PATH_SUFFIXES lib)
+    MESSAGE(STATUS "Found libamdhip64:    ${LIB_AMD_HIP}")
+    MESSAGE(STATUS "Found libhsa-runtime: ${LIB_HSA_RUNTIME}")
+    IF(NOT LIB_AMD_HIP OR NOT LIB_HSA_RUNTIME)
+      MESSAGE(FATAL_ERROR "Could not find AMD HIP and/or HSA Runtime library(ies)")
+    ENDIF()


I (personally) don't like replicating a find_package in-place. I think it complicates the flow of our CMake. Can we move this to stuff to a local cmake/Modules/FindAMDGPU.cmake script?

jjwilke · 2021-03-16T02:58:57Z

cmake/kokkos_arch.cmake

+  ELSE()
+    SET(AMDGPU_ARCH_FLAG "--offload-arch")
+    GLOBAL_APPEND(KOKKOS_AMDGPU_OPTIONS -x hip)
+    GLOBAL_APPEND(KOKKOS_AMDGPU_OPTIONS --rocm-path=$ENV{ROCM_PATH})


Agreed with @jrmadsen. Referencing the comment below, I think it makes sense to have a FindROCM.cmake file that resolves all these paths. ROCM_PATH would then be a variable.

jjwilke · 2021-03-16T03:02:31Z

cmake/kokkos_arch.cmake

@@ -414,6 +433,8 @@ FUNCTION(CHECK_AMDGPU_ARCH ARCH FLAG)
      IF(KOKKOS_ENABLE_HIP_RELOCATABLE_DEVICE_CODE)
        GLOBAL_APPEND(KOKKOS_LINK_OPTIONS "${AMDGPU_ARCH_FLAG}=${FLAG}")
      ENDIF()
+      #GLOBAL_APPEND(KOKKOS_LINK_OPTIONS -lamdhip64 -lhsa-runtime64 -L${LIB_AMD_HIP_DIR})
+      GLOBAL_APPEND(KOKKOS_LINK_OPTIONS ${LIB_HSA_RUNTIME} ${LIB_AMD_HIP})


Continuing with the find module approach, Kokkos is structured around defining imported or interface targets (e.g. Kokkos::ROCM) that would have an INTERFACE_LINK_LIBRARIES=amdhip64 hsa-runtime64 and you would have a target_link_libraries(kokkoscore PUBLIC Kokkos::ROCM)

jjwilke · 2021-03-16T03:04:08Z

cmake/kokkos_compiler_id.cmake

@@ -153,7 +157,7 @@ ELSEIF(KOKKOS_CXX_COMPILER_ID STREQUAL NVIDIA)
    MESSAGE(FATAL_ERROR "${KOKKOS_MESSAGE_TEXT}")
  ENDIF()
  SET(CMAKE_CXX_EXTENSIONS OFF CACHE BOOL "Kokkos turns off CXX extensions" FORCE)
-ELSEIF(KOKKOS_CXX_COMPILER_ID STREQUAL HIP)
+ELSEIF(KOKKOS_CXX_COMPILER_ID STREQUAL HIPCC)


Any reason for the change from HIP to HIPCC?

Because effectively this tells whether hipcc is being used as the compiler. "hip" does not designate any compiler.

Rombur · 2021-03-16T18:56:16Z

This PR does not work for me. Depending on the version of HIP I am using I get different errors either at configuration time or at compile time.

dalg24 · 2021-03-16T20:40:24Z

it turns out find_package(hip) is broken For ROCm 3.8->4.0

Rombur · 2021-03-17T12:59:25Z

cmake/Modules/FindTPLROCM.cmake

+
+kokkos_create_imported_tpl(ROCM INTERFACE
+  LINK_LIBRARIES ${HSA_RUNTIME_LIBRARY} ${AMD_HIP_LIBRARY}
+  COMPILE_OPTIONS -D__HIP_ROCclr__


Shouldn't this be a COMPILE_DEFINITIONS?

Yes you are right for classic cmake but kokkos_create_imported_tpl does not currently support it probably because target_compile_definitions was only added in 3.11

kokkos/cmake/kokkos_functions.cmake

Lines 310 to 314 in 233fc53

CMAKE_PARSE_ARGUMENTS(TPL

"INTERFACE"

"LIBRARY"

"LINK_LIBRARIES;INCLUDES;COMPILE_OPTIONS;LINK_OPTIONS"

${ARGN})

jjwilke

I'm not able to test, but looks good to me. It would be nice to add the compile definitions to the TPL system correctly... but I guess not necessary.

dalg24 · 2021-03-18T01:02:41Z

I'm not able to test, but looks good to me. It would be nice to add the compile definitions to the TPL system correctly... but I guess not necessary.

Will implement in a follow up PR

abbotts reviewed Mar 11, 2021

View reviewed changes

cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved

abbotts reviewed Mar 11, 2021

View reviewed changes

cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved

dalg24 requested a review from Rombur March 11, 2021 17:30

dalg24 added 5 commits March 12, 2021 13:33

Set KOKKOS_CXX_COMPILER_ID to Cray instead of HIP

1cf5b98

Set proper arch flag for Cray Clang and append required flags to comp…

bcbbc54

…ile HIP

Prefer --offload-arch to --cuda-gpu-arch when using Cray Clang

59d7c1a

Fixup detect if hipcc is being used

9e04f7b

KOKKOS_CXX_COMPILER_ID={HIP -> HIPCC}

5c422be

dalg24 force-pushed the hip_support_cray_clang branch from eb9f5d1 to 5c422be Compare March 12, 2021 19:33

dalg24 added 2 commits March 12, 2021 14:28

Find libamdhip64 and libhsa-runtime64

6b2ed5b

Guard HIP specific logic

07a643e

dalg24 force-pushed the hip_support_cray_clang branch from a6a0cff to 07a643e Compare March 12, 2021 21:01

jrmadsen suggested changes Mar 13, 2021

View reviewed changes

jjwilke suggested changes Mar 16, 2021

View reviewed changes

dalg24 added 3 commits March 16, 2021 13:02

Define ROCM TPL

2227ae4

Defer finding amdhip64 lib and compile/link flags to find_package

5964ce2

FIxup

d625b40

dalg24 added 2 commits March 16, 2021 15:35

Find amdhip64 and hsa-runtime64 intead of using find_pacakge(hip)

342b1cd

Fixup --rocm-path

c693379

jrmadsen approved these changes Mar 17, 2021

View reviewed changes

Rombur approved these changes Mar 17, 2021

View reviewed changes

dalg24 marked this pull request as ready for review March 17, 2021 15:12

jjwilke approved these changes Mar 17, 2021

View reviewed changes

dalg24 mentioned this pull request Mar 18, 2021

Add support for compile definition to the TPL system #3858

Closed

dalg24 merged commit f0c7101 into kokkos:develop Mar 18, 2021

dalg24 deleted the hip_support_cray_clang branch March 18, 2021 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable HIP with Cray Clang #3842

Enable HIP with Cray Clang #3842

dalg24 commented Mar 11, 2021 •

edited

dalg24 commented Mar 12, 2021

jrmadsen left a comment

jrmadsen Mar 13, 2021

jjwilke Mar 16, 2021

jrmadsen Mar 13, 2021

jjwilke Mar 16, 2021

jjwilke Mar 16, 2021

jjwilke Mar 16, 2021

dalg24 Mar 16, 2021

Rombur commented Mar 16, 2021

dalg24 commented Mar 16, 2021

Rombur Mar 17, 2021

dalg24 Mar 17, 2021

jjwilke left a comment

dalg24 commented Mar 18, 2021

	CMAKE_PARSE_ARGUMENTS(TPL
	"INTERFACE"
	"LIBRARY"
	"LINK_LIBRARIES;INCLUDES;COMPILE_OPTIONS;LINK_OPTIONS"
	${ARGN})

Enable HIP with Cray Clang #3842

Enable HIP with Cray Clang #3842

Conversation

dalg24 commented Mar 11, 2021 • edited

dalg24 commented Mar 12, 2021

jrmadsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rombur commented Mar 16, 2021

dalg24 commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjwilke left a comment

Choose a reason for hiding this comment

dalg24 commented Mar 18, 2021

dalg24 commented Mar 11, 2021 •

edited