Support HIP/ROCm backends for GPUs #101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ture-rocm-port
benson31
added
enhancement
review requested
hip rocm
benson31
force-pushed
the
feature-rocm-port
branch
from
cddce8d
to
ebabe95
Compare
ndryden
reviewed
May 6, 2020
ndryden
approved these changes
May 7, 2020
timmoon10
reviewed
May 8, 2020
timmoon10
requested changes
May 9, 2020
timmoon10
approved these changes
May 10, 2020
bvanessen
approved these changes
Jun 4, 2020
There was a problem hiding this comment.
LGTM. I see that there are still some unaddressed comments from @timmoon10
Co-authored-by: Tim Moon <moon13@llnl.gov>
…ture-rocm-port
Co-authored-by: Tim Moon <moon13@llnl.gov>
…ntal into feature-rocm-port
…ture-rocm-port
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most of the GPU calls have been factored out into clean HIP vs CUDA backends. Even though HIP is a thin layer over CUDA on NVIDIA platforms, we don't use massive portions of the API and it seemed that there could be an advantage to having the two separate. (Additionally, I don't want CUDA users to have to install ROCm on systems that don't need it just to get back to CUDA.) Moreover, I could envision an optimization for one platform being neutral or even bad for the other, so keeping the two isolated will keep the optimization paths independent.
Similarly, cuBLAS and rocBLAS have been separated as the two have surprisingly divergent APIs. This is abstracted behind the
gpu_blasnamespace.This port supports Aluminum in both CUDA mode and HIP mode.
I have not tested with hipCUB yet, but this support should work.hipCUB support has been added and seems fine.I have not tested with GPU half types under HIP; please review this PR as-is and I will work on that functionality. If it makes it in before this merges, super. Otherwise, I can do a follow-on PR.
As part of this refactor, the preprocessing macros have changed slightly.
HYDROGEN_HAVE_GPUshould now be used to protect any generic GPU-specific code.HYDROGEN_HAVE_CUDAandHYDROGEN_HAVE_ROCMshould be used to protect code that is GPU-backend specific, for CUDA and HIP/ROCm, respectively. Cleaning this up accounts for a large portion of the changes in this PR.As a final note here: the
SyncInfoobject changed slightly. Instead of a struct with public event and stream, this is now a class that usesEvent()andStream()to access the event and stream handles, respectively. This is another large portion of the changes in the PR.