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.

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

Properly locate libomp.so when installed in clang target subdirectory. #840

Merged

Conversation

estewart08
Copy link
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. Use find_package(LLVM) to provide PATHS to 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.

Summary of proposed changes:
-If compiler is gcc, look for ROCm llvm to resolve libomp.so
-If compiler is hipcc, use find_package(OpenMP).
-Allow proper detection of new lib installation subdirectory x86_64-unknown-linux-gnu

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. Use find_package(LLVM)
to provide PATHS to 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.
@estewart08
Copy link
Contributor Author

I think this is a better approach to #839.

Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

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

Okay if it passes seems good to me. @daineAMD you want to review as well

@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 8, 2024

Looks like CMake errors on LLVM, you can add some message status lines to help you debug

@estewart08
Copy link
Contributor Author

It's complaining about not finding libffi and the C language not being enabled. But it looks like it grabbed the wrong llvm cmake config. I added a second commit. Do I need to retrigger the checks, or does that happen automatically?

@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 8, 2024

Every commit will trigger another build and test

@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 8, 2024

CMake Error at /opt/cmake-3.25.2/share/cmake-3.25/Modules/Internal/CheckSourceCompiles.cmake:44 (message):
check_source_compiles: C: needs to be enabled before use.

You can add C to LANGUAGES for the hipblas-clients project it should be fine.

Copy link
Contributor

@daineAMD daineAMD left a comment

Choose a reason for hiding this comment

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

CI compiles with g++ typically. There is a multi-compiler job that would test out hipcc. After some discussion with Sam it sounds like it was broken from the org. change, but it should start running now; so please wait until that passes.

If it doesn't get picked up, I opened PR #841 so you can specify the compiler CI uses once it's merged.

@estewart08
Copy link
Contributor Author

@daineAMD Can you confirm whether or not these failures are related to this patch. The multi-compiler build has undefined references to the thread library, which I did not change.

@TorreZuk
Copy link
Contributor

TorreZuk commented Apr 9, 2024

Those errors can be seen in develop branch but @estewart08 but that means your changes aren't getting tested either on that build.

@daineAMD
Copy link
Contributor

daineAMD commented Apr 9, 2024

Working out some problems with hipcc on CI. Tested with --compiler=hipcc with my local ROCm 6.0 and it passed tests, so fine to merge by me and I'll work on getting multi-compiler in a good state.

@estewart08 estewart08 merged commit e2ad580 into ROCm:develop Apr 9, 2024
18 of 22 checks passed
@estewart08 estewart08 deleted the llvm-enable-per-target-runtime-dir-v2 branch April 9, 2024 17:33
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