Add rpath for hipfft-test to help find libomp. by malcolmroberts · Pull Request #173 · ROCm/hipFFT

malcolmroberts · 2025-07-22T16:26:53Z

No description provided.

TorreZuk · 2025-07-23T16:31:46Z

rocBLAS and hipBLAS are now using this scheme ROCm/rocm-libraries#624

malcolmroberts · 2025-07-23T19:53:40Z

rocBLAS and hipBLAS are now using this scheme ROCm/rocm-libraries#624

@TorreZuk : it looks to me like roc/hipBLAS won't be able to find all of the libraries that it needs if one installs to, for example, ~/rocBLAS. Am I mistaken about this? Has this been tested?

TorreZuk · 2025-07-23T20:55:31Z

rocBLAS and hipBLAS are now using this scheme ROCm/rocm-libraries#624

@TorreZuk : it looks to me like roc/hipBLAS won't be able to find all of the libraries that it needs if one installs to, for example, ~/rocBLAS. Am I mistaken about this? Has this been tested?

Here is the hipBLAS version tested with gcc and clang ROCm/hipBLAS#1050. Tests reported along with normal CI and some manual tests (inc. windows). Are you installing the clients to some unique location? If so, there is runpath information still being set. This change prefers the cmake config for openmp amd clang that was @estewart08 addition to rocm.

malcolmroberts · 2025-07-23T22:05:00Z

rocBLAS and hipBLAS are now using this scheme ROCm/rocm-libraries#624

@TorreZuk : it looks to me like roc/hipBLAS won't be able to find all of the libraries that it needs if one installs to, for example, ~/rocBLAS. Am I mistaken about this? Has this been tested?

Here is the hipBLAS version tested with gcc and clang ROCm/hipBLAS#1050. Tests reported along with normal CI and some manual tests (inc. windows). Are you installing the clients to some unique location? If so, there is runpath information still being set. This change prefers the cmake config for openmp amd clang that was @estewart08 addition to rocm.

I'm looking at the use-case where the installation directory isn't /opt/rocm. We have use cases where users will install rocFFT to their home directory, so, yes, the clients may be installed to some unique location.

estewart08 · 2025-07-23T22:56:28Z

rocBLAS and hipBLAS are now using this scheme ROCm/rocm-libraries#624

@TorreZuk : it looks to me like roc/hipBLAS won't be able to find all of the libraries that it needs if one installs to, for example, ~/rocBLAS. Am I mistaken about this? Has this been tested?

Here is the hipBLAS version tested with gcc and clang ROCm/hipBLAS#1050. Tests reported along with normal CI and some manual tests (inc. windows). Are you installing the clients to some unique location? If so, there is runpath information still being set. This change prefers the cmake config for openmp amd clang that was @estewart08 addition to rocm.

I'm looking at the use-case where the installation directory isn't /opt/rocm. We have use cases where users will install rocFFT to their home directory, so, yes, the clients may be installed to some unique location.

ROCm strongly prefers relative paths for rpath from what I understand. If there are install locations outside of a ROCm directory structure then I would think LD_LIBRARY_PATH would need set.

malcolmroberts · 2025-07-23T22:56:47Z

Here is the hipBLAS version tested with gcc and clang ROCm/hipBLAS#1050. Tests reported along with normal CI and some manual tests (inc. windows). Are you installing the clients to some unique location? If so, there is runpath information still being set. This change prefers the cmake config for openmp amd clang that was @estewart08 addition to rocm.

I'm looking at the use-case where the installation directory isn't /opt/rocm. We have use cases where users will install rocFFT to their home directory, so, yes, the clients may be installed to some unique location.

Working with

set_target_properties( ${target} PROPERTIES BUILD_RPATH "${HIP_CLANG_ROOT}/lib" )
set_target_properties( ${target} PROPERTIES INSTALL_RPATH "$ORIGIN/../llvm/lib" )

and installing to a user-local directory, the installed version of hipff-test doesn't find libomp.so.

regan-amd

Thank you!

evetsso · 2025-07-24T15:49:57Z

Rpath is being used for multiple purposes:

Finding libhipfft.so
Finding runtime libraries (libamdhip64.so)
Finding libomp.so

If any of these are installed to different prefixes, then a single rpath entry won't be enough to find all of them. Also, we don't want absolute install rpaths, as @estewart08 points out. Build rpaths may be absolute.

So we can have ${HIP_CLANG_ROOT}/lib as a build rpath, but install rpath will need to be something relative to $ORIGIN. If different libraries are in different prefixes, then IMO it's reasonable to expect users to have to change their configuration to make them findable by the dynamic linker. Rpath isn't really meant to solve this whole problem. Users could either change /etc/ld.so.conf.d or set LD_LIBRARY_PATH.

If absolute install rpaths are to be avoided, I don't think the current version of this PR is aligned with that

Fix linking directory to fix clients compilation. Allow for building from clients/tests.

malcolmroberts · 2025-07-28T22:09:45Z

Ok, in the current version, tests installed to non-rocm directories need LD_LIBRARY_PATH set for libomp.so to be found. For the upcoming OpenMP configuration, we can use openmp_LIB_DIR to help with linking.

Also, hipfft.h is changed from including hipfft-export.h to including hipfft/hipfft-export.h, which fixes the test-only build.

regan-amd · 2025-07-31T01:24:05Z

-#include "hipfft-export.h"
-#include "hipfft-version.h"
+#include "hipfft/hipfft-export.h"
+#include "hipfft/hipfft-version.h"


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?

regan-amd · 2025-07-31T01:24:29Z

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


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

regan-amd · 2025-07-31T01:29:17Z

 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 )


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)?

regan-amd · 2025-07-31T01:38:27Z

+  #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 )


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?

regan-amd · 2025-07-31T01:48:17Z

+    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}" )


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

ammallya · 2025-07-31T18:44:51Z

Imported to ROCm/rocm-libraries

@malcolmroberts

…anges for rocm-libraries CI (#1004) --- 🔁 Imported from [ROCm/hipFFT#173](ROCm/hipFFT#173) 🧑‍💻 Originally authored by @malcolmroberts --------- Co-authored-by: Malcolm Roberts <malcolm.roberts@amd.com> Co-authored-by: Malcolm Roberts <malcolm.i.w.roberts@gmail.com>

@malcolmroberts

…anges for rocm-libraries CI (#1004) --- 🔁 Imported from [ROCm/hipFFT#173](ROCm/hipFFT#173) 🧑‍💻 Originally authored by @malcolmroberts --------- Co-authored-by: Malcolm Roberts <malcolm.roberts@amd.com> Co-authored-by: Malcolm Roberts <malcolm.i.w.roberts@gmail.com>

…ories cmake clause (#1381) I believe such conditionals cannot be used within `target_include_directories`. Current build lines on `develop` (when building hipfft's and its clients) are polluted, _e.g._ see the parts between `**` delimiters in the copy-pasted following example: `/opt/rocm/llvm/bin/amdclang++ -DUSE_HIPRAND -DUSE_PROF_API=1 -D__HIP_PLATFORM_AMD__=1 -I/src/rocm-libraries/projects/hipfft/clients/tests/**if -I"/src/rocm-libraries/projects/hipfft/clients/tests/(" -I/src/rocm-libraries/projects/hipfft/clients/tests/HIPFFT_BUILD_SCOPE -I"/src/rocm-libraries/projects/hipfft/clients/tests/)" -I/src/rocm-libraries/projects/hipfft/clients/tests/../../library/include -I/src/rocm-libraries/projects/hipfft/clients/tests/endif** -I/src/rocm-libraries/projects/hipfft/library/include -I/src/rocm-libraries/projects/hipfft/build/include/hipfft -I/src/rocm-libraries/projects/hipfft/build/include [...]` I also think it's not necessary to explicitly add the intended inclusion to the `cmake` logic, as mentioned in [that comment](ROCm/hipFFT#173 (comment)).

@malcolmroberts

…anges for rocm-libraries CI (#1004) --- 🔁 Imported from [#173](#173) 🧑‍💻 Originally authored by @malcolmroberts --------- Co-authored-by: Malcolm Roberts <malcolm.roberts@amd.com> Co-authored-by: Malcolm Roberts <malcolm.i.w.roberts@gmail.com>

…ories cmake clause (#1381) I believe such conditionals cannot be used within `target_include_directories`. Current build lines on `develop` (when building hipfft's and its clients) are polluted, _e.g._ see the parts between `**` delimiters in the copy-pasted following example: `/opt/rocm/llvm/bin/amdclang++ -DUSE_HIPRAND -DUSE_PROF_API=1 -D__HIP_PLATFORM_AMD__=1 -I/src/rocm-libraries/projects/hipfft/clients/tests/**if -I"/src/rocm-libraries/projects/hipfft/clients/tests/(" -I/src/rocm-libraries/projects/hipfft/clients/tests/HIPFFT_BUILD_SCOPE -I"/src/rocm-libraries/projects/hipfft/clients/tests/)" -I/src/rocm-libraries/projects/hipfft/clients/tests/../../library/include -I/src/rocm-libraries/projects/hipfft/clients/tests/endif** -I/src/rocm-libraries/projects/hipfft/library/include -I/src/rocm-libraries/projects/hipfft/build/include/hipfft -I/src/rocm-libraries/projects/hipfft/build/include [...]` I also think it's not necessary to explicitly add the intended inclusion to the `cmake` logic, as mentioned in [that comment](#173 (comment)).

Add rpath for hipfft-test to help find libomp.

f41cd65

malcolmroberts requested review from af-ayala, eng-flavio-teixeira, evetsso and regan-amd as code owners July 22, 2025 16:26

regan-amd reviewed Jul 23, 2025

View reviewed changes

Comment thread clients/tests/CMakeLists.txt

Comment thread clients/tests/CMakeLists.txt Outdated

Define static class member of hipfft_params for hipfft_mpi_worker

6df6ab6

Use INSTALL_RPATH_USE_LINK_PATH for linking test executables.

2fdaaba

malcolmroberts requested a review from regan-amd July 24, 2025 01:15

regan-amd previously approved these changes Jul 24, 2025

View reviewed changes

openmp_LIB_DIR style rpath for tests when OpenMP CONFIG works.

d1c8157

Fix linking directory to fix clients compilation. Allow for building from clients/tests.

malcolmroberts requested a review from a team as a code owner July 28, 2025 22:07

malcolmroberts requested a review from regan-amd July 28, 2025 22:07

malcolmroberts mentioned this pull request Jul 29, 2025

Add rpath for rocff-test to help find libomp.so ROCm/rocFFT#628

Closed

Make clients/tests find hipfft only of not in hipfft build scope

0dac5f5

regan-amd reviewed Jul 31, 2025

View reviewed changes

assistant-librarian Bot mentioned this pull request Jul 31, 2025

Add rpath for rocfft-test and hipfft-test to help find libomp, and changes for rocm-libraries CI ROCm/rocm-libraries#1004

Merged

ammallya closed this Jul 31, 2025

regan-amd mentioned this pull request Aug 27, 2025

[hipFFT] Remove conditional from body of test's target_include_directories cmake clause ROCm/rocm-libraries#1381

Merged

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

Conversation

malcolmroberts commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

TorreZuk commented Jul 23, 2025

Uh oh!

malcolmroberts commented Jul 23, 2025

Uh oh!

TorreZuk commented Jul 23, 2025

Uh oh!

malcolmroberts commented Jul 23, 2025

Uh oh!

estewart08 commented Jul 23, 2025

Uh oh!

malcolmroberts commented Jul 23, 2025

Uh oh!

regan-amd left a comment

Choose a reason for hiding this comment

Uh oh!

evetsso commented Jul 24, 2025

Uh oh!

malcolmroberts commented Jul 28, 2025

Uh oh!

regan-amd Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

regan-amd Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

regan-amd Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

regan-amd Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

regan-amd Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ammallya commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

regan-amd Jul 31, 2025 •

edited

Loading

regan-amd Jul 31, 2025 •

edited

Loading