-
Notifications
You must be signed in to change notification settings - Fork 120
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
Adding ROCm support for AMD GPUs #638
Conversation
No change on usage for NVIDIA CUDA. $ USE_GPU=Yes pip install . For HIP, USE_HIP needs to be added as follows. $ USE_GPU=Yes USE_HIP=Yes pip install .
Verbose log outputs for pip install and CMakeis temporalily specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the first PR!
- I'm not sure about AMD HIP, but the APIs of both looks alike. I think wrapping some types and functions of both architecture like you are doing with
GTYPE
is better than adding so muchifdef
s and typing almost same codes. - I'm thinking of using
cudaOccupancyMaxPotentialBlockSize
to adapt to many NVIDIA GPUs in Flexible and Effienct Blocksize and Loopdim #628 . Is there any functions like this in HIP?
Thanks for your review!
|
|
|
Wrapping types and functions was applied for CUDA and HIP. For this purpose, I created (CC: @ckime-amd) |
Thanks for the notification @hisohara, looks like a good contrib to enable AMD GPU support. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for defining alias.
It looks great.
I have one change-request.
src/gpusim/CMakeLists.txt
Outdated
@@ -1,4 +1,43 @@ | |||
cmake_minimum_required(VERSION 3.0) | |||
project(qulacs LANGUAGES HIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When building on CUDA GPU, this line causes the following error.
-- The HIP compiler identification is unknown
CMake Error at /usr/share/cmake-3.22/Modules/CMakeDetermineHIPCompiler.cmake:102 (message):
Failed to find ROCm root directory.
Call Stack (most recent call first):
src/gpusim/CMakeLists.txt:2 (project)
Thanks for taking look at it again. I should have realized earlier.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #638 +/- ##
==========================================
+ Coverage 85.98% 88.13% +2.15%
==========================================
Files 127 137 +10
Lines 13253 16273 +3020
Branches 1695 2174 +479
==========================================
+ Hits 11396 14343 +2947
- Misses 1824 1837 +13
- Partials 33 93 +60 ☔ View full report in Codecov by Sentry. |
|
Thanks for your approval!
|
Thank you. Now it's reflected. |
Adopt hipOccupancyMaxPotentialBlockSize()
Let me update current status. When Meanwhile, hard coding like the following is acceptable?
|
Please ignore my last comment. I've found the workaround with
I've confirmed no major performance impact. At QCBMopt4 with 25 qubits, it shows slightly faster by 2.4%. Hope that this change is acceptable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. That seems nice.
Thanks for your approval! |
@hisohara I'm sorry, could you run |
I updated |
Format is broken while merging main branch. |
I executed
Could you please tell me what file is broken on your side? |
My using clang-format was 14.0.0 on Ubuntu 22.04, while GitHub Action uses 10.0-50. I changed it to 11.1.0-6 which is the oldest version available for Ubuntu 22.04. It could apply src/cppsim/circuit.cpp where the test was failed before. Hope that the issue is resolved.. |
Hi, please let me know if there is anything from my side to proceed. |
Sorry for late reply. |
Finally! Thanks so much for your continuous support and accepting this big change. I'm very happy to contribute to this project. |
This PR adds the support of ROCm with HIP for AMD GPUs. CUDA codes are converted to HIP API. Per the past discussion with maintainer,
#ifdef __HIP_PLATFORM_AMD__
is used for HIP API. This HIP region is called only when HIP compiler is used. No change on CUDA codes.To compile for ROCm,
USE_HIP=Yes
needs to be added onUSE_GPU=Yes
as follows:$ USE_GPU=Yes USE_HIP=Yes pip install .
Tested hardware environment:
MI250, MI300A
Tested software environment:
ROCm 6.0.2 and ROCm 6.1
References