Skip to content

Add rpath for hipfft-test to help find libomp. #173

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

Closed
malcolmroberts wants to merge 5 commits into from
Closed

Add rpath for hipfft-test to help find libomp. #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f41cd65
Add rpath for hipfft-test to help find libomp.
malcolmroberts Jul 22, 2025
6df6ab6
Define static class member of hipfft_params for hipfft_mpi_worker
regan-amd Jul 23, 2025
2fdaaba
Use INSTALL_RPATH_USE_LINK_PATH for linking test executables.
malcolmroberts Jul 23, 2025
d1c8157
openmp_LIB_DIR style rpath for tests when OpenMP CONFIG works.
malcolmroberts Jul 28, 2025
0dac5f5
Make clients/tests find hipfft only of not in hipfft build scope
malcolmroberts Jul 30, 2025
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
51 changes: 36 additions & 15 deletions clients/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ else( )
"Install path prefix, prepended onto install directories" )
endif( )

# Dependencies

find_package( ROCmCMakeBuildTools REQUIRED CONFIG PATHS /opt/rocm )
include(ROCMInstallTargets)
list( APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../cmake )


# This has to be initialized before the project() command appears
# Set the default of CMAKE_BUILD_TYPE to be release, unless user
# specifies with -D. MSVC_IDE does not use CMAKE_BUILD_TYPE
Expand All @@ -42,13 +49,19 @@ endif()

project( hipfft-clients-tests LANGUAGES CXX )

if( NOT HIPFFT_BUILD_SCOPE )
find_package( hipfft REQUIRED CONFIG PATHS )
endif()

find_package( Boost REQUIRED)

set( Boost_USE_STATIC_LIBS OFF )


find_package( FFTW 3.0 REQUIRED MODULE COMPONENTS FLOAT DOUBLE )

set( BUILD_WITH_LIB "ROCM" CACHE STRING "Build ${PROJECT_NAME} with ROCM or CUDA libraries" )

set( THREADS_PREFER_PTHREAD_FLAG ON )
find_package( Threads REQUIRED )

Expand Down Expand Up @@ -84,7 +97,7 @@ if( NOT BUILD_WITH_LIB STREQUAL "CUDA" )
if( WIN32 )
find_package( HIP CONFIG REQUIRED )
else()
find_package( HIP MODULE REQUIRED )
find_package( hip REQUIRED CONFIG PATHS /opt/rocm/lib/cmake/hip/ )
endif()
endif()

Expand All @@ -98,6 +111,14 @@ endif()
string( CONCAT TESTS_OUT_DIR "${PROJECT_BINARY_DIR}" ${TESTS_OUT_DIR} )

option( BUILD_CLIENTS_TESTS_OPENMP "Build tests with OpenMP" ON )
if( BUILD_CLIENTS_TESTS_OPENMP )
Copy link
Copy Markdown
Contributor

@regan-amd regan-amd Jul 31, 2025

Choose a reason for hiding this comment

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

Suggestion: maybe we could add AND BUILD_WITH_LIB NOT STREQUAL "CUDA" above, to avoid a fatal build error if building with cuda (openMP is ignored thereafter in that case, anyways)?

# Attempt to find a config version, which provides openmp_LIB_DIR.
#find_package( OpenMP CONFIG PATHS "${HIP_CLANG_ROOT}/lib/cmake" )
if( NOT OPENMP_FOUND OR NOT DEFINED ${openmp_LIB_DIR} )
# Fall-back to module mode.
find_package( OpenMP REQUIRED )
Copy link
Copy Markdown
Contributor

@regan-amd regan-amd Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would this ever define openmp_LIB_DIR? If or when it does, is the ${HIP_CLANG_ROOT}/${openmp_LIB_DIR} used below consistent with it regardless of which OpenMP package is found?

endif()
endif()

foreach( target ${TEST_TARGETS} )
set_target_properties( ${target} PROPERTIES
Expand Down Expand Up @@ -140,20 +161,6 @@ foreach( target ${TEST_TARGETS} )
target_compile_definitions( ${target} PUBLIC _CUFFT_BACKEND )
endif()

if( BUILD_CLIENTS_TESTS_OPENMP )
find_package(OpenMP REQUIRED)
if( BUILD_WITH_LIB STREQUAL "CUDA" )
message( STATUS "OpenMP is not supported on CUDA, building tests without it" )
else()
target_compile_options( ${target} PRIVATE -DBUILD_CLIENTS_TESTS_OPENMP )
if(NOT (CMAKE_CXX_COMPILER MATCHES ".*hipcc$" OR CMAKE_CXX_COMPILER MATCHES ".*clang\\+\\+"))
target_link_libraries( ${target} PRIVATE OpenMP::OpenMP_CXX )
target_include_directories( ${target} PRIVATE ${HIP_CLANG_ROOT}/include )
else()
target_link_libraries( ${target} PRIVATE OpenMP::OpenMP_CXX )
endif()
endif()
endif()

target_include_directories( ${target}
PRIVATE
Expand All @@ -168,6 +175,18 @@ foreach( target ${TEST_TARGETS} )
hip::hipfft
${FFTW_LIBRARIES}
)

if( BUILD_CLIENTS_TESTS_OPENMP )
if( BUILD_WITH_LIB STREQUAL "CUDA" )
message( STATUS "OpenMP is not supported on CUDA, building tests without it" )
else()
if( DEFINED ${openmp_LIB_DIR} )
set_target_properties( ${target} PROPERTIES BUILD_RPATH "${HIP_CLANG_ROOT}/${openmp_LIB_DIR}" )
set_target_properties( ${target} PROPERTIES INSTALL_RPATH "${HIP_CLANG_ROOT}/${openmp_LIB_DIR}" )
Copy link
Copy Markdown
Contributor

@regan-amd regan-amd Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would this ever set the binary's install rpath as a relative path, i.e., would ${HIP_CLANG_ROOT} ever be a relative path?

endif()
target_link_libraries( ${target} PRIVATE OpenMP::OpenMP_CXX )
endif()
endif()

if( HIPFFT_MPI_ENABLE )
target_link_libraries( ${target}
Expand All @@ -184,6 +203,8 @@ foreach( target ${TEST_TARGETS} )
rocm_install(TARGETS ${target} COMPONENT tests)
endforeach()

find_package( GTest 1.11.0 )

if( GTEST_FOUND )
target_include_directories( hipfft-test PRIVATE $<BUILD_INTERFACE:${GTEST_INCLUDE_DIRS}> )
target_link_libraries( hipfft-test PRIVATE ${GTEST_LIBRARIES} )
Expand Down
3 changes: 3 additions & 0 deletions clients/tests/hipfft_mpi_worker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
#include "../../shared/mpi_worker.h"
#include "../hipfft_params.h"

// initialize static class member of hipfft_params
std::vector<gpubuf> hipfft_params::externally_managed_workareas = std::vector<gpubuf>();

Comment on lines +26 to +28
Copy link
Copy Markdown
Contributor

@regan-amd regan-amd Jul 31, 2025

Choose a reason for hiding this comment

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

FYI: this may be removed as that was resolved in #174

int main(int argc, char* argv[])
{
return mpi_worker_main<std::array<hipfft_params, 1>, false>(
Expand Down
4 changes: 2 additions & 2 deletions library/include/hipfft/hipfft.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@
#define DISABLE_WARNING_RETURN_TYPE
#endif

#include "hipfft-export.h"
#include "hipfft-version.h"
#include "hipfft/hipfft-export.h"
#include "hipfft/hipfft-version.h"
Comment on lines -48 to +49
Copy link
Copy Markdown
Contributor

@regan-amd regan-amd Jul 31, 2025

Choose a reason for hiding this comment

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

I am wary of this change. In my understanding, once installed, hipfft.h, hipfft-export.h, and hipfft-version.h are all placed in the same directory, the last two files being automatically created when building/installing the hipfft library (they are not natively in the repository). So, for anything that includes an installed library's hipfft.h, the compiler should typically find the required hipfft-export.h, and hipfft-version.h (without any change to this file) since the directory of that included hipfft.h is the first place it should explore when using such double quotes #include (reference).

I'm actually suspecting that the test-only build failed because it was not including an installed library's hipfft.h (say /opt/rocm/include/hipfft/hipfft.h) but your local repository's header instead, due to
target_include_directories( ${target} PRIVATE [...] $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../library/include> )
being used (unconditionally of HIPFFT_BUILD_SCOPE) in clients/tests/CMakeLists.txt.
In fact, I do not think that we need to add that include directory explicitly: if hipfft is built/installed alongside the tests, cmake should use the built/installed library's header automatically; if doing a test-only build, cmake should find the installed library's header from the target_link_libraries( ${target} PRIVATE hip::hipfft [...]) command (given how the public target_include_directories of hipfft itself are defined).

Could we try reverting this file's changes and removing $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../library/include> from the tests' target_include_directories instead?

#include <hip/hip_complex.h>
#include <hip/library_types.h>

Expand Down