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

Support CUDA-only builds #2

Closed
wants to merge 1 commit into from
Closed

Support CUDA-only builds #2

wants to merge 1 commit into from

Conversation

upsj
Copy link

@upsj upsj commented Feb 22, 2021

hipFFT currently doesn't build on a system with only HIP-CUDA installed, since it uses the hip config package instead of the HIP module package (which was quite confusing when I ran into this issue)

This PR is a WIP attempt to fix the build issues as I go along integrating hipFFT into Ginkgo

The CUDA-specific code is right now shamelessly adapted from the corresponding hipBLAS code, but maybe there should be a more consistent way to deal with the ROCm/CUDA distinction.

@upsj upsj marked this pull request as draft February 22, 2021 20:18
@memmett
Copy link
Contributor

memmett commented Feb 22, 2021

Hi @upsj, thanks for looking into this! We would like to make building on CUDA platforms as smooth as possible.

We are making some changes for the CUDA build for our internal process. You might see some changes in the CMake files shortly. Hopefully our changes don't conflict with each other (much).

Looking forward to working with you to improve the CUDA builds.

@upsj upsj marked this pull request as ready for review February 25, 2021 10:29
@upsj upsj changed the title WIP: Support CUDA-only builds Support CUDA-only builds Feb 25, 2021
@upsj
Copy link
Author

upsj commented Feb 25, 2021

I can now confirm that the builds work on a variety of build configurations with both CUDA and ROCm (modulo rocFFT bugs), probably needs some formatting, though

@memmett
Copy link
Contributor

memmett commented Mar 1, 2021

Hi @upsj, thanks again for this PR. I incorporated some of these changes and then added logic to avoid requiring the ROCm CMake infrastructure if ROCm isn't found. This avoids having to download it :)

Can you verify that these changes work for you?

We'll have to figure out how to get you proper recognition...

@upsj
Copy link
Author

upsj commented Mar 3, 2021

I tried to build hipFFT in the same settings again, but it seems like the latest changes broke the install target, which is no longer present, probably due to the removed CMake infrastructure you mentioned.

And no worries, I don't care much about recognition for this, just happy to have everything work correctly :)

@upsj
Copy link
Author

upsj commented Apr 24, 2021

Hi @memmett, I just wanted to follow up on this, it looks like the aforementioned issue is still present in the current version:

> mkdir build && cmake -DBUILD_WITH_LIB=CUDA .. && make install
make: *** No rule to make target 'install'.  Stop.

@malcolmroberts
Copy link
Contributor

Hi, I was actually just working on this. I can build with cuFFT backend using g++ as the compiler. This is in commit bf5bd2e, which was merged yesterday. There still seems to be an issue when compiling with hipcc and using cuFFT as the backend.

Would you mind giving this a try?

@malcolmroberts malcolmroberts self-requested a review April 24, 2021 20:34
@upsj
Copy link
Author

upsj commented Apr 25, 2021

Hi @malcolmroberts, thanks for the quick response. The issue is more related to CMake configuration than actual compilation (the latter works fine on a system without ROCm installed). The PR itself is actually no longer relevant, since the issues were fixed in the last few commits, but this broke the installation step on CUDA without ROCm present:
https://github.com/ROCmSoftwarePlatform/hipFFT/blob/4030407eb768137720c3d0b52806da6cbba11713/library/CMakeLists.txt#L110
The install target is only available if ROCm was found. So this is just to let you know that support for CUDA is currently conditional on ROCm being installed, which is maybe not the most general solution possible?

@malcolmroberts
Copy link
Contributor

After a bunch of cmake changes, I think that this should be working now. This should be fixed in 894ea5c. I've tested with g++ and hip-nvcc for compilers.

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