Skip to content

Fix finding LAPACK and CBLAS #987

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

benrichard-amd
Copy link

When building hipBLAS and not linking BLIS or providing NETLIB LAPACK and CBLAS in ${BUILD_DIR}/deps/deps-install it is assumed that NETLIB installed to the system should be used. While find_package() is used, the variables it provides are not used and instead the library names are set to the ones of the NETLIB reference implementation even though when find_package() points to a different implementation.

This needs to be patched to allow other LAPACK / CBLAS providers like OpenBLAS.

Note: This is part of a set of patches required to get TheRock (https://github.com/ROCm/TheRock) to build properly.

@TorreZuk
Copy link
Contributor

Probably have to review that our build from source is fine with this change in other build environments but this old fallback should be avoided as AOCL is required for proper API testing: https://rocm.docs.amd.com/projects/hipBLAS/en/latest/install/Linux_Install_Guide.html#dependencies-for-building-library

@stellaraccident
Copy link
Contributor

Probably have to review that our build from source is fine with this change in other build environments but this old fallback should be avoided as AOCL is required for proper API testing: https://rocm.docs.amd.com/projects/hipBLAS/en/latest/install/Linux_Install_Guide.html#dependencies-for-building-library

Was there a question/suggestion there? @marbre who authored this patch originally.

Every component team has odd requirements for building, and for product-wide distribution, they don't matter. TheRock and the portable linux subset it targets presently bundles OpenBLAS. Is there a reason why that should not be supported?

@TorreZuk
Copy link
Contributor

AMD's AOCL is the standard ILP64 and LP64 CPU reference library for our test suite. Is there a reason you can not use it? It has both Linux and Windows distributions as documented. If you are suggesting OpenBLAS should also be supported for ILP64 reference we will have to plan support.

@stellaraccident
Copy link
Contributor

AMD's AOCL is the standard ILP64 and LP64 CPU reference library for our test suite. Is there a reason you can not use it? It has both Linux and Windows distributions as documented. If you are suggesting OpenBLAS should also be supported for ILP64 reference we will have to plan support.

We'll use OpenBLAS as the default because it is well known and an easy dependency to satisfy for portable builds. We can discuss adding a proprietary BLAS dep as an option, but it won't be the default.

@TorreZuk
Copy link
Contributor

@stellaraccident what is your test coverage using OpenBLAS? AMD AOCL is simple to install and should have good GEMM performance.

@stellaraccident
Copy link
Contributor

@stellaraccident what is your test coverage using OpenBLAS? AMD AOCL is simple to install and should have good GEMM performance.

We are working to to all tests project wide.

AOCL is proprietary and covered by a EULA. We won't publish it by default as part of a portable build, to say nothing of needing to satisfy arm64 builds.

It is perfectly fine for the local project to enable as an option, but we can't have a rocm level hard dependency on our own x86 blas library.

We're also not going to include a default in any of our project wide builds that requires a click through EULA.

@stellaraccident
Copy link
Contributor

Feel free to fork this conversation internally between you/me/Dan if you'd like to discuss further offline.

Copy link
Contributor

@amcamd amcamd left a comment

Choose a reason for hiding this comment

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

We will keep with using AOCL if it is installed on the machine, and fallback to OpenBLAS. We have not recently compared the time to run all tests with AOCL VS OpenBLAS, we have also not run all correctness tests, including the large size tests for ILP64 functionality to see if they pass with OpenBLAS.

@stellaraccident
Copy link
Contributor

We will keep with using AOCL if it is installed on the machine, and fallback to OpenBLAS. We have not recently compared the time to run all tests with AOCL VS OpenBLAS, we have also not run all correctness tests, including the large size tests for ILP64 functionality to see if they pass with OpenBLAS.

No worries - we'll work with you on the OpenBLAS based testing if it causes problems. I understand this is the less invested path.

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.

4 participants