Skip to content

Add ROCm support. #393

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

Closed
wants to merge 9 commits into from
Closed

Add ROCm support. #393

wants to merge 9 commits into from

Conversation

acai66
Copy link

@acai66 acai66 commented Jul 7, 2020

note:

I have modified some CUDA apis to HIP apis, so i can compile it with ROCm. I have test mmcv/tests/test_ops/test_bbox.py and test_nms.py, both of them work fine.

image

But i don't have all the tests, and i have only an AMD graphic card, so i can not test it with CUDA.
It needs to be tested enough Before merging to main stream.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #393 into master will increase coverage by 3.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   62.42%   65.51%   +3.09%     
==========================================
  Files         126      121       -5     
  Lines        7116     6540     -576     
  Branches     1255     1141     -114     
==========================================
- Hits         4442     4285     -157     
+ Misses       2469     2053     -416     
+ Partials      205      202       -3     
Flag Coverage Δ
#unittests 65.51% <ø> (+3.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/cnn/bricks/non_local.py 93.75% <0.00%> (-5.39%) ⬇️
mmcv/ops/nms.py 11.22% <0.00%> (-2.94%) ⬇️
mmcv/ops/sync_bn.py 17.24% <0.00%> (-1.89%) ⬇️
mmcv/runner/base_runner.py 65.98% <0.00%> (-0.85%) ⬇️
mmcv/utils/progressbar.py 74.54% <0.00%> (-0.23%) ⬇️
mmcv/runner/__init__.py 100.00% <0.00%> (ø)
mmcv/runner/hooks/sync_buffer.py
mmcv/runner/fp16_utils.py
mmcv/onnx/__init__.py
mmcv/onnx/onnx_utils/symbolic_helper.py
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1830347...66df97a. Read the comment docs.

__syncwarp();
#endif
#ifdef __HIP_PLATFORM_HCC__
// confused !!!
Copy link

@wangyingrui wangyingrui Sep 2, 2020

Choose a reason for hiding this comment

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

__syncwarp() in order to avoid read-write conflict of output_val, when warpReduceSum reading output_val from other threads within a warp.
AMD HIP doesn't support __syncwarp() now. Using __syncthreads() instead is ok, although bringing a few performance decrease.

at::cuda::CUDAGuard device_guard(bboxes1.device());
cudaStream_t stream = at::cuda::getCurrentCUDAStream();
#endif
#ifdef __HIP_PLATFORM_HCC__
// at::cuda::HIPGuard device_guard(bboxes1.device());

Choose a reason for hiding this comment

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

Why comment out HIPGuard?

Copy link
Author

Choose a reason for hiding this comment

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

Because pytorch handles ROCm as CUDA, it will cause assert device type error if not comment.
I'm not familiar with CUDA or ROCm, but i hope ROCM can get official efficient support.

Copy link

@wangyingrui wangyingrui Sep 7, 2020

Choose a reason for hiding this comment

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

It seems that here should use HIPGuardImplMasqueradingAsCUDA.h and getCurrentHIPStreamMasqueradingAsCUDA?

@zhouzaida
Copy link
Collaborator

closed by duplicate PR #1022

@zhouzaida zhouzaida closed this Jun 29, 2021
@OpenMMLab-Assistant003
Copy link

Hi @acai66!We are grateful for your efforts in helping improve this open-source project during your personal time.
Welcome to join OpenMMLab Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA
If you have a WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤ @acai66

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