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

Simplify finding the hip package #66

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Simplify finding the hip package #66

merged 1 commit into from
Jul 31, 2023

Conversation

trixirt
Copy link
Contributor

@trixirt trixirt commented Jul 21, 2023

On Fedora, where hip is installed as an rpm, its cmake files can not be found and are reported as an error.

CMake Error at cmake/dependencies.cmake:44 (find_package):
No "FindHIP.cmake" found in CMAKE_MODULE_PATH.

This change treats hip as a the normal package.

On Fedora, where hip is installed as an rpm, its cmake files can
not be found and are reported as an error.

CMake Error at cmake/dependencies.cmake:44 (find_package):
  No "FindHIP.cmake" found in CMAKE_MODULE_PATH.

This change treats hip as a the normal package.

Signed-off-by: Tom Rix <trix@redhat.com>
@malcolmroberts
Copy link
Contributor

Thanks for this PR! Looks like a good idea; we'll take a look and run it through the tests.

Copy link
Contributor

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

In the currently released versions of HIP, hip-config.cmake only supports the AMD platform. You must use the FindHIP.cmake module when building for the NVIDIA platform. However, this proposed change still does the module-mode search first and hip-config.cmake should eventually support the NVIDIA platform. So, I think this is fine.

I'm hoping to move away from FindHIP.cmake entirely. We'll probably want to clean up this whole section of HIP-searching code in the near future. It's a bit overly-complicated at the moment.

@malcolmroberts
Copy link
Contributor

@cgmb - we could also add some CI tests to check that this solves the use case described by @trixirt . Might be a lot of docker images to cover. Any thoughts?

@cgmb
Copy link
Contributor

cgmb commented Jul 26, 2023

@cgmb - we could also add some CI tests to check that this solves the use case described by @trixirt . Might be a lot of docker images to cover. Any thoughts?

I wouldn't worry about it. @trixirt and I can confirm this change is useful for Fedora and Debian, respectively. You can validate that it doesn't break the official ROCm builds, and that is probably sufficient.

Our whole method of using FindHIP.cmake is kind of silly. We're using FindHIP.cmake from the HIP package, so you need to find HIP to use FindHIP.cmake. Writing a bunch of tests might help prevent a few bugs, but I think it would be cheaper in the long run to eliminate the complexity that causes these sorts of bugs.

@heroxbd
Copy link

heroxbd commented Jul 29, 2023

This commit is also useful to Gentoo.

By the cmake documentation,

Do not write find modules for packages that themselves build with
CMake. Instead provide a CMake package configuration file with the
package itself.

HIP uses cmake to build. It should not have shipped a FindHIP.cmake (a cmake MODULE).

You must use the FindHIP.cmake module when building for the NVIDIA platform.

For nVidia targets HIP is a wrapper around CUDA, it should hide the implementation detail of itself to hipFFT.

FindCUDA.cmake may be incorporated during the build of HIP.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jul 30, 2023
Loosen the versioned dependence on dev-util/hip.

Reference: ROCm/hipFFT#66
Signed-off-by: Benda Xu <heroxbd@gentoo.org>
@evetsso evetsso merged commit c2b06a3 into ROCm:develop Jul 31, 2023
@cgmb
Copy link
Contributor

cgmb commented Jul 31, 2023

Hi @heroxbd,

HIP uses cmake to build. It should not have shipped a FindHIP.cmake (a cmake MODULE).

Just FYI, the folks that work on libraries like hipFFT are users of HIP, much like you. They work for AMD, but not on HIP itself. While they certainly can relay concerns to the HIP team, it's usually better for affected users to file issues directly on the repo for the component that the problem originates from. It helps prevent duplicate reports, it's less work for the library teams, and it helps keep you in the loop on any updates.

@heroxbd
Copy link

heroxbd commented Aug 1, 2023

eam, it's usually be

Thanks @cgmb! Sure, we are sorting out the patches carried by Gentoo hip package and report them to the HIP team.

@cgmb
Copy link
Contributor

cgmb commented Aug 1, 2023

Thank you for doing that. I know it's a fair bit of effort to upstream things, but it is very much appreciated.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Nov 20, 2023
hipFFT is not updated in 5.7.1, and we need to manually backport it.

Reference: ROCm/hipFFT#66
Signed-off-by: Benda Xu <heroxbd@gentoo.org>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Dec 14, 2023
According to the cmake documentation, do not write find modules for
packages that themselves build with CMake. Instead provide a CMake
package configuration file with the package itself.

FindHIP is needed by HIP over CUDA, not what Gentoo is aiming for.

Reference: ROCm/hipFFT#66
Reference: https://cmake.org/cmake/help/latest/command/find_package.html
Bug: ROCm/hipamd#39
Signed-off-by: Benda Xu <heroxbd@gentoo.org>
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.

5 participants