Skip to content

Add CMake HIP language support #740

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

Merged
merged 7 commits into from
May 29, 2025
Merged

Conversation

cgmb
Copy link
Collaborator

@cgmb cgmb commented Jun 13, 2024

Add USE_HIPCXX option to build using CMake's support for the HIP language. This option requires CMake 3.21.3 or newer.

The rocsolver clients shouldn't need to be built as HIP, but they require rocblas_is_complex<T>, which is only provided by the rocblas headers when compiling as HIP. In the future, this could be corrected.

Note that set_source_files_properties only affects the targets declared in the same CMake file as the call to set the properties. As such, it is difficult to use together with target_sources. All uses of latter have therefore been removed. The project declaration in the clients directory has also been removed so that PROJECT_SOURCE_DIR can always be used to refer to the root directory.

@cgmb cgmb added noOptimizations Disable optimized kernels for small sizes for some routines noExtendedCI Disable extended tests labels Jun 13, 2024
Add USE_HIPCXX option to build using CMake's support for the
HIP language. This option requires CMake 3.21.3 or newer.

The rocsolver clients shouldn't need to be built as HIP, but they
require rocblas_is_complex, which is only provided by the rocblas
headers when compiling as HIP. In the future, this could be
corrected.

Note that set_source_files_properties only affects the targets declared
in the same CMake file as the call to set the properties. As such, it
is difficult to use together with target_sources. All uses of latter
have therefore been removed. The project declaration in the clients
directory has also been removed so that PROJECT_SOURCE_DIR can always
be used to refer to the root directory.
@cgmb cgmb force-pushed the use-cmake-hiplang branch from 957b4fb to c228674 Compare April 29, 2025 21:23
@cgmb cgmb marked this pull request as ready for review April 29, 2025 21:24
cgmb added 3 commits April 29, 2025 23:26
The project() call has been removed from clients/CMakeLists.txt, so the
only project() call is at the rocsolver root. The references to the
PROJECT_BINARY_DIR in that file can be replaced by
CMAKE_CURRENT_BINARY_DIR.
@cgmb cgmb enabled auto-merge (squash) April 30, 2025 22:27
@cgmb
Copy link
Collaborator Author

cgmb commented Apr 30, 2025

This should not change anything about the rocsolver library unless users pass -DUSE_HIPCXX=ON when building it. The approach here is the same as used for rocALUTION in ROCm 6.0:
ROCm/rocALUTION@5bd014f

This was my test setup in an Ubuntu 24.04 docker container:

apt-get -y update
apt-get -y upgrade
apt-get -y install gnupg2 wget sudo
sudo mkdir --parents --mode=0755 /etc/apt/keyrings
wget https://repo.radeon.com/rocm/rocm.gpg.key -O - | gpg --dearmor | sudo tee /etc/apt/keyrings/rocm.gpg > /dev/null
echo "deb [arch=amd64 signed-by=/etc/apt/keyrings/rocm.gpg] https://repo.radeon.com/rocm/apt/6.4 noble main" | sudo tee --append /etc/apt/sources.list.d/rocm.list
echo -e 'Package: *\nPin: release o=repo.radeon.com\nPin-Priority: 600' | sudo tee /etc/apt/preferences.d/rocm-pin-600
apt-get -y update
apt-get -y install rocm-dev rocblas-dev rocsparse-dev rocprim-dev build-essential cmake git libfmt-dev gfortran libopenblas-dev libgtest-dev
git clone https://github.com/cgmb/rocSOLVER.git -b use-cmake-hiplang
cd rocSOLVER
HIPCXX=/opt/rocm/llvm/bin/amdclang++ cmake -S. -Bbuild \
  -DCMAKE_HIP_ARCHITECTURES=gfx906 \
  -DBUILD_TESTING=ON \
  -DROCSOLVER_FIND_PACKAGE_LAPACK_CONFIG=OFF \
  -DUSE_HIPCXX=ON
make -j16 -C build

Note that with -DUSE_HIPCXX=ON, the default build target will be the user's GPU, not the full list of all architectures that rocSOLVER supports. I think that's the default we eventually want to move to, so it will be nice to be able to have that behaviour accessible via this non-default build configuration.

@cgmb
Copy link
Collaborator Author

cgmb commented May 1, 2025

@haampie, this is pretty much what I'm going to suggest for the various rocm libs. At some point, we can then make it the default. I'd also like to collect the flags used for AMD's official ROCm releases (e.g., CMAKE_INSTALL_PATH, CMAKE_INSTALL_RPATH, CMAKE_HIP_ARCHITECTURES, etc.), and move them into a centralized location that defines the flags for the builds of the packages that AMD provides.

Copy link
Collaborator

@tfalders tfalders left a comment

Choose a reason for hiding this comment

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

LGTM!

@tfalders
Copy link
Collaborator

tfalders commented May 28, 2025

Test failure is unrelated. I'm forcing the merge.
EDIT: Oh, wait, the potential conflict is still there. I'll hold off for now.

@tfalders
Copy link
Collaborator

Forcing the merge (for real this time)

@tfalders tfalders disabled auto-merge May 29, 2025 13:55
@tfalders tfalders merged commit 9edf5e4 into ROCm:develop May 29, 2025
13 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
noExtendedCI Disable extended tests noOptimizations Disable optimized kernels for small sizes for some routines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants