Skip to content

Revamp how libomp.so is found#853

Closed
estewart08 wants to merge 3 commits into
ROCm:developfrom
estewart08:llvm-enable-per-target-runtime-dir-fallback
Closed

Revamp how libomp.so is found#853
estewart08 wants to merge 3 commits into
ROCm:developfrom
estewart08:llvm-enable-per-target-runtime-dir-fallback

Conversation

@estewart08
Copy link
Copy Markdown
Contributor

An upstream llvm change enables
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default for the openmp build. This installs the openmp libraries into
/opt/rocm-ver/llvm/lib/x86_64-unknown-linux-gnu instead of /opt/rocm-ver/llvm/lib. Currenty, hipBLAS only uses /lib.

Since hipBLAS uses gcc by default, find_package(OpenMP) will locate libgomp from the gnu installation. We would prefer to use libomp from the ROCm install. Query amdclang to find LLVM_TARGET_TRIPLE and use as a suffix for find_library(omp).

On the other hand, the user can choose to use hipcc, which allows find_package(OpenMP) to properly locate libomp inside ROCm llvm.

To include legacy support, I left the hardcoded -L HIP_CLANG_ROOT as a fallback.

This would be much simpler if hipBLAS defaulted to hipcc as the cmake compiler.

Summary of proposed changes:

  • If compiler is gcc, query amdclang for LLVM_TARGET_TRIPLE and use as suffix for find_library(omp).
  • If compiler is hipcc, use find_package(OpenMP).
  • If cmake cannot find libomp.so then fallback to using -L HIP_CLANG_ROOT/lib.
  • Allow proper detection of new lib installation subdirectory (i.e. x86_64-unknown-linux-gnu).
  • Removed dependency on find_package(LLVM) due to rocm-llvm-dev/devel not being installed by default on various operating systems, which would not have the cmake config files.

An upstream llvm change enables
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default for the openmp
build. This installs the openmp libraries into
/opt/rocm-ver/llvm/lib/x86_64-unknown-linux-gnu instead of
/opt/rocm-ver/llvm/lib. Currenty, hipBLAS only uses /lib.

Since hipBLAS uses gcc by default, find_package(OpenMP) will
locate libgomp from the gnu installation. We would prefer
to use libomp from the ROCm install. Query amdclang to find
LLVM_TARGET_TRIPLE and use as a suffix for find_library(omp).

On the other hand, the user can choose to use hipcc, which
allows find_package(OpenMP) to properly locate libomp
inside ROCm llvm.

To include legacy support, I left the hardcoded -L with
use of HIP_CLANG_ROOT as a fallback.

This would be much simpler if hipBLAS defaulted to hipcc
as the cmake compiler.
Comment thread clients/CMakeLists.txt
Comment on lines -121 to -122
if (TARGET OpenMP::OpenMP_CXX)
set( COMMON_LINK_LIBS "OpenMP::OpenMP_CXX")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So aren't we missing the OpenMP::OpenMP_CXX link request on CUDA backend?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, I can move set( COMMON_LINK_LIBS ${LIBOMP_FOUND}) outside of the amd guard.

Comment thread clients/CMakeLists.txt
# 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( CMAKE_CXX_COMPILER MATCHES ".*/hipcc$" )
find_package(OpenMP)
Copy link
Copy Markdown
Contributor

@TorreZuk TorreZuk Apr 22, 2024

Choose a reason for hiding this comment

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

Similar question we shouldn't require NON hipcc to find_package(OpenMP) so can this just be moved two lines above? Sorry I meant other than hipcc we still want find_package to set cmake target etc.

Copy link
Copy Markdown
Contributor Author

@estewart08 estewart08 Apr 22, 2024

Choose a reason for hiding this comment

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

When the cmake compiler is hipcc find_package finds the correct ROCm libomp.so. Otherwise, gcc will get libgomp.so from gcc installation. I would rather get the libomp from ROCm to be consistent. With your current configuration cmake adds libgomp.so with -L/rpaths -lomp to the ROCm library. So I am fairly certain libgomp is ignored.

Remove amd check for rpath addtion. If we use the libomp.so from
ROCm we need the rpath.
@TorreZuk
Copy link
Copy Markdown
Contributor

Hi @estewart08 are you still active on this?

@estewart08
Copy link
Copy Markdown
Contributor Author

I am going to pick this effort back up, so I would prefer it to stay open for now.

@TorreZuk
Copy link
Copy Markdown
Contributor

@estewart08 if picking this up again for next release time is ticking....

@TorreZuk
Copy link
Copy Markdown
Contributor

@estewart08 you mentioned you were going to continue this work. Any ETA?

@estewart08
Copy link
Copy Markdown
Contributor Author

I am currently looking into providing an openmp-config.cmake file that will provide the proper location of the library. No definitive ETA at the moment.

@TorreZuk
Copy link
Copy Markdown
Contributor

TorreZuk commented May 5, 2025

I am currently looking into providing an openmp-config.cmake file that will provide the proper location of the library. No definitive ETA at the moment.

@estewart08 @stellaraccident This would be a big help as we are now seeing issues with tighter redhat 10 rpath restrictions so avoiding the workaround by getting full cmake config could solve newer problems.

@stellaraccident
Copy link
Copy Markdown
Contributor

I am currently looking into providing an openmp-config.cmake file that will provide the proper location of the library. No definitive ETA at the moment.

@estewart08 @stellaraccident This would be a big help as we are now seeing issues with tighter redhat 10 rpath restrictions so avoiding the workaround by getting full cmake config could solve newer problems.

I don't have full state on the set of constraints that need to be solved, but I've definitely developed... Let's call it experience... On this issue and agree we need to improve the situation. Agreed that some form of standalone cmake config for this seems like the right answer. If we want to look at details, maybe look at this in upcoming office hours.

@estewart08 estewart08 closed this Jul 7, 2025
@estewart08
Copy link
Copy Markdown
Contributor Author

Closed in favor of: #1038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants