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 CMake HIP language support #266

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+68 −40
Open

Add CMake HIP language support #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e22792
Add ROCRAND_CMAKE_HIP_LANGUAGE option
cgmb Apr 1, 2022
1a1bb3b
Leave architecture selection up to CMake
cgmb Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 40 additions & 32 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# MIT License
#
# Copyright (c) 2018-2021 Advanced Micro Devices, Inc. All rights reserved.
# Copyright (c) 2018-2022 Advanced Micro Devices, Inc. All rights reserved.
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -66,39 +66,44 @@ set(BUILD_SHARED_LIBS ON CACHE BOOL "Build shared")
# Include cmake scripts
include(cmake/Dependencies.cmake)

# Detect compiler support for target ID
# This section is deprecated. Please use rocm_check_target_ids for future use.
if( CMAKE_CXX_COMPILER MATCHES ".*/hipcc$" )
execute_process(COMMAND ${CMAKE_CXX_COMPILER} "--help"
OUTPUT_VARIABLE CXX_OUTPUT
OUTPUT_STRIP_TRAILING_WHITESPACE
ERROR_STRIP_TRAILING_WHITESPACE)
string(REGEX MATCH ".mcode\-object\-version" TARGET_ID_SUPPORT ${CXX_OUTPUT})
endif()
option(ROCRAND_CMAKE_HIP_LANGUAGE "Use CMake's HIP language support" OFF)
if(ROCRAND_CMAKE_HIP_LANGUAGE)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO CUDA and HIP language support should be uniformly handled in rocRAND, so similar to other projects I would rather have an option like ROCRAND_LANGUAGE (the name is not important) which could be set to either HIP or CUDA.
If this variable is not set then the old behaviour applies based on the C++ compiler set (hipcc or clang -> HIP without cmake language support, gcc or nvcc -> CUDA), if set it overwrites this detection and forces either HIP or CUDA cmake language.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting thought. I'll change this behaviour from ROCRAND_CMAKE_HIP_LANGUAGE=ON to ROCRAND_CMAKE_LANG=HIP. The default would be ROCRAND_CMAKE_LANG=CXX, and if somebody wants to implement CMake CUDA language support, it would be ROCRAND_CMAKE_LANG=CUDA.

enable_language(HIP)
Copy link

Choose a reason for hiding this comment

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

You still need to call find_package(hip) when using hip language. Libraries like rocRAND need to provide hip::host in the targets interface because it includes hip's runtime API headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I was just about to request your review. I haven't yet tested this with downstream libraries.

Copy link

Choose a reason for hiding this comment

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

Ah wait it looks like rocRAND actually needs hip::device as it includes hip's device headers:

https://github.com/ROCmSoftwarePlatform/rocRAND/blob/develop/library/include/rocrand/rocrand.h#L29

We would need two different targets to support using rocRAND with HIP language and with CXX language.

Copy link
Contributor Author

@cgmb cgmb Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's not actually clear to me why it includes <hip/hip_runtime.h>. rocRAND appears to provide a C99 API. Sadly, the documentation is... lacking.

EDIT: Oh, I see. rocrand.h is a C API but rocrand.hpp clearly does need <hip/hip_runtime.h>.

Copy link
Contributor Author

@cgmb cgmb Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We would need two different targets to support using rocRAND with HIP language and with CXX language.

Maybe we could detect the language with a generator expression? I'm not entirely sure it's possible, but I was thinking something like:

set_property(TARGET roc::rocrand APPEND PROPERTY
  INTERFACE_LINK_LIBRARIES "$<$<NOT:$<COMPILE_LANGUAGE:HIP>>:hip::device>"
)

else()
# Detect compiler support for target ID
# This section is deprecated. Please use rocm_check_target_ids for future use.
if( CMAKE_CXX_COMPILER MATCHES ".*/hipcc$" )
execute_process(COMMAND ${CMAKE_CXX_COMPILER} "--help"
OUTPUT_VARIABLE CXX_OUTPUT
OUTPUT_STRIP_TRAILING_WHITESPACE
ERROR_STRIP_TRAILING_WHITESPACE)
string(REGEX MATCH ".mcode\-object\-version" TARGET_ID_SUPPORT ${CXX_OUTPUT})
endif()

set( AMDGPU_TARGETS "all" CACHE STRING "Compile for which gpu architectures?")
# Set the AMDGPU_TARGETS
rocm_check_target_ids(DEFAULT_AMDGPU_TARGETS
TARGETS "gfx803;gfx900:xnack-;gfx906:xnack-;gfx908:xnack-;gfx90a:xnack-;gfx90a:xnack+;gfx1030"
)
if (AMDGPU_TARGETS)
if( AMDGPU_TARGETS STREQUAL "all" )
set( gpus "${DEFAULT_AMDGPU_TARGETS}")
else()
set( gpus "${AMDGPU_TARGETS}")
endif()
# must FORCE set this AMDGPU_TARGETS before any find_package( hip ...), in this file
# to override CACHE var and set --offload-arch flags via hip-config.cmake hip::device dependency
set( AMDGPU_TARGETS "${gpus}" CACHE STRING "AMD GPU targets to compile for" FORCE )
endif()
set( AMDGPU_TARGETS "all" CACHE STRING "Compile for which gpu architectures?")
# Set the AMDGPU_TARGETS
rocm_check_target_ids(DEFAULT_AMDGPU_TARGETS
TARGETS "gfx803;gfx900:xnack-;gfx906:xnack-;gfx908:xnack-;gfx90a:xnack-;gfx90a:xnack+;gfx1030"
)
if (AMDGPU_TARGETS)
if( AMDGPU_TARGETS STREQUAL "all" )
set( gpus "${DEFAULT_AMDGPU_TARGETS}")
else()
set( gpus "${AMDGPU_TARGETS}")
endif()
# must FORCE set this AMDGPU_TARGETS before any find_package( hip ...), in this file
# to override CACHE var and set --offload-arch flags via hip-config.cmake hip::device dependency
set( AMDGPU_TARGETS "${gpus}" CACHE STRING "AMD GPU targets to compile for" FORCE )
endif()

# Verify that hcc compiler is used on ROCM platform
# TODO: Fix VerifyCompiler for Windows
if (NOT WIN32)
include(cmake/VerifyCompiler.cmake)
else()
list(APPEND CMAKE_PREFIX_PATH ${ROCM_PATH} $ENV{ROCM_PATH}/hip $ENV{ROCM_PATH}/llvm)
find_package(hip REQUIRED CONFIG PATHS ${HIP_DIR} $ENV{ROCM_PATH})
# Verify that hcc compiler is used on ROCM platform
# TODO: Fix VerifyCompiler for Windows
if (NOT WIN32)
include(cmake/VerifyCompiler.cmake)
else()
list(APPEND CMAKE_PREFIX_PATH ${ROCM_PATH} $ENV{ROCM_PATH}/hip $ENV{ROCM_PATH}/llvm)
find_package(hip REQUIRED CONFIG PATHS ${HIP_DIR} $ENV{ROCM_PATH})
endif()
endif()

# Build option to disable -Werror
Expand All @@ -108,6 +113,9 @@ option(DISABLE_WERROR "Disable building with Werror" ON)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_HIP_STANDARD 11)
set(CMAKE_HIP_STANDARD_REQUIRED ON)
set(CMAKE_HIP_EXTENSIONS OFF)
if(DISABLE_WERROR)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -mf16c")
else()
Expand Down
9 changes: 7 additions & 2 deletions benchmark/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
# Get sources
file(GLOB tmp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_rocrand*.cpp)
set(rocRAND_BENCHMARK_SRCS ${tmp})
if(HIP_COMPILER STREQUAL "nvcc")
if(ROCRAND_CMAKE_HIP_LANGUAGE)
set_source_files_properties(${rocRAND_BENCHMARK_SRCS}
PROPERTIES
LANGUAGE HIP
)
elseif(HIP_COMPILER STREQUAL "nvcc")
file(GLOB tmp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_curand*.cpp)
set(rocRAND_BENCHMARK_SRCS ${rocRAND_BENCHMARK_SRCS} ${tmp})
endif()
Expand All @@ -29,7 +34,7 @@ foreach(benchmark_src ${rocRAND_BENCHMARK_SRCS})
else()
target_link_libraries(${BENCHMARK_TARGET}
rocrand
hip::device
$<$<NOT:$<BOOL:ROCRAND_CMAKE_HIP_LANGUAGE>>:hip::device>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for benchmarks an if statement is used, but here its a generator expression. I would stick to the if in both places for consistency, and because this setting is known at configure time, a generator expression is not required.

)
endif()
set_target_properties(${BENCHMARK_TARGET}
Expand Down
17 changes: 12 additions & 5 deletions library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,18 @@ if(HIP_COMPILER STREQUAL "nvcc")
)
set(CUDA_HOST_COMPILER ${CMAKE_CXX_COMPILER})
else()
target_link_libraries(rocrand PRIVATE hip::device)
if(NOT WIN32)
foreach(amdgpu_target ${AMDGPU_TARGETS})
target_link_libraries(rocrand PRIVATE --amdgpu-target=${amdgpu_target})
endforeach()
if(ROCRAND_CMAKE_HIP_LANGUAGE)
set_source_files_properties(${rocRAND_SRCS}
PROPERTIES
LANGUAGE HIP
)
else()
target_link_libraries(rocrand PRIVATE hip::device)
if(NOT WIN32)
foreach(amdgpu_target ${AMDGPU_TARGETS})
target_link_libraries(rocrand PRIVATE --amdgpu-target=${amdgpu_target})
endforeach()
endif()
Maetveis marked this conversation as resolved.
Show resolved Hide resolved
endif()
endif()

Expand Down
10 changes: 9 additions & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ endfunction()

# Get rocRAND tests source files
file(GLOB rocRAND_TEST_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/*.cpp)
if(ROCRAND_CMAKE_HIP_LANGUAGE)
set_source_files_properties(${rocRAND_TEST_SRCS}
PROPERTIES
LANGUAGE HIP
)
endif()
Comment on lines +22 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have these lines together with the lines for CUDA a few lines below.


# Build rocRAND tests
foreach(test_src ${rocRAND_TEST_SRCS})
Expand Down Expand Up @@ -52,7 +58,9 @@ foreach(test_src ${rocRAND_TEST_SRCS})
rocrand
)
if(HIP_COMPILER STREQUAL "hcc" OR HIP_COMPILER STREQUAL "clang")
target_link_libraries(${test_name} hip::device)
if(NOT ROCRAND_CMAKE_HIP_LANGUAGE)
target_link_libraries(${test_name} hip::device)
endif()
endif()
set_target_properties(
${test_name}
Expand Down