Skip to content

Fix OpenMP and link tools against hip::host #1352

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

Merged
marbre merged 1 commit into from
Aug 27, 2025
Merged

Fix OpenMP and link tools against hip::host #1352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 35 additions & 26 deletions projects/rocsparse/clients/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,22 @@
cmake_minimum_required(VERSION 3.5 FATAL_ERROR)

function( apply_omp_settings lib_target_ )
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::OpenMP_CXX)
set_target_properties( ${lib_target_} PROPERTIES
BUILD_RPATH "${HIP_CLANG_ROOT}/lib"
)
set_target_properties( ${lib_target_} PROPERTIES
INSTALL_RPATH "$ORIGIN/../llvm/lib"
)
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::omp)
set_target_properties( ${lib_target_} PROPERTIES
BUILD_RPATH "${HIP_CLANG_ROOT}/${openmp_LIB_DIR}"
)
set_target_properties( ${lib_target_} PROPERTIES
INSTALL_RPATH "$ORIGIN/../llvm/${openmp_LIB_DIR}"
)
if(NOT WIN32)
Copy link
Copy Markdown
Contributor

@jsandham jsandham Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The if(NOT WIN32) and if(WIN32) changes should probably be propagated to rocblas etc as they do the same thing I think. Doesn't have to be done here, just mentioning it so other reviewers might see.

Copy link
Copy Markdown
Member Author

@marbre marbre Aug 27, 2025

Choose a reason for hiding this comment

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

rocBLAS will switch to next-cmake rather sooner than later which re-implements the current build logic. I am confident they have taken care and we didn't had similar build issues even with the old config so far.

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::OpenMP_CXX)
set_target_properties( ${lib_target_} PROPERTIES
BUILD_RPATH "${HIP_CLANG_ROOT}/lib"
)
set_target_properties( ${lib_target_} PROPERTIES
INSTALL_RPATH "$ORIGIN/../llvm/lib"
)
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::omp)
set_target_properties( ${lib_target_} PROPERTIES
BUILD_RPATH "${HIP_CLANG_ROOT}/${openmp_LIB_DIR}"
)
set_target_properties( ${lib_target_} PROPERTIES
INSTALL_RPATH "$ORIGIN/../llvm/${openmp_LIB_DIR}"
)
endif()
endif()
endfunction()

Expand Down Expand Up @@ -87,21 +89,28 @@ if ( BUILD_FORTRAN_CLIENTS )

endif()

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# Look for openmp config in ROCm install to populate openmp_LIB_DIR and openmp_LIB_INSTALL_DIR
find_package(OpenMP CONFIG PATHS "${HIP_CLANG_ROOT}/lib/cmake")
endif()

# OpenMP
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::omp)
set(ROCSPARSE_CLIENT_LIBS "OpenMP::omp")
message(STATUS "Found openmp-config.cmake at ${OpenMP_DIR}")
if(NOT WIN32)
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# Look for openmp config in ROCm install to populate openmp_LIB_DIR and openmp_LIB_INSTALL_DIR
find_package(OpenMP CONFIG PATHS "${HIP_CLANG_ROOT}/lib/cmake")
endif()

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND TARGET OpenMP::omp)
set(ROCSPARSE_CLIENT_LIBS "OpenMP::omp")
message(STATUS "Found openmp-config.cmake at ${OpenMP_DIR}")
else()
# if it fails to find OpenMP compile and link flags in strange configurations it can just use non-parallel reference computation
# if there is no omp.h to find the client compilation will fail and this should be obvious, used to be REQUIRED
find_package(OpenMP)
if (TARGET OpenMP::OpenMP_CXX)
set(ROCSPARSE_CLIENT_LIBS "OpenMP::OpenMP_CXX")
endif()
endif()
else()
# if it fails to find OpenMP compile and link flags in strange configurations it can just use non-parallel reference computation
# if there is no omp.h to find the client compilation will fail and this should be obvious, used to be REQUIRED
find_package(OpenMP)
if (TARGET OpenMP::OpenMP_CXX)
set(ROCSPARSE_CLIENT_LIBS "OpenMP::OpenMP_CXX")
if(OPENMP_FOUND)
set(ROCSPARSE_CLIENT_LIBS "libomp")
Copy link
Copy Markdown
Contributor

@jsandham jsandham Aug 27, 2025

Choose a reason for hiding this comment

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

Do we know why this doesn't seem to be required in rocblas?

Copy link
Copy Markdown
Member Author

@marbre marbre Aug 27, 2025

Choose a reason for hiding this comment

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

I haven't looked closely to the logic applied in rocBLAS and rather compared with the changes that broke the Windows build. Before that problematic patch, Windows was linking to libomp if OpenMP was found which I re-introduced here.

endif()
endif()

Expand Down
3 changes: 3 additions & 0 deletions projects/rocsparse/clients/tools/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ foreach(app ${ROCSPARSEIO_TOOLS_SOURCES})
# Internal common header
target_include_directories(${app} PRIVATE $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../include> $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../common>)

# Target link libraries
target_link_libraries(${app} PRIVATE hip::host)

# Set tools output directory
set_target_properties(${app} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/staging")

Expand Down