Add support of std::atomic for hip-clang #764
Conversation
| */ | ||
|
|
||
| /* HIT_START | ||
| * BUILD: %t %s ../test_common.cpp |
There was a problem hiding this comment.
I think you need EXCLUDE_HIP_PLATFORM nvcc here since calling std::atomic* is not supported by nvcc I believe.
|
@yxsamliu unless there is an application ask for this, I would avoid doing it since it violates ODR (we're seldom if ever symmetric in what regards lockfree-ness with the host e.g. we don't do sub-DWORD atomics) and since it will stellarly break for non-lockfree cases, since we absolutely cannot get to the sharded lock table. As far as I know CUDA does not do this, so why are we trying to add this to hip-clang? Has there been some recent change in CUDA? |
|
There are some recent changes in TensorFlow which requires a framework called rccl, which uses std::atomic in device code. HCC is able to compile that. This change just allows hip-clang to do the same thing like HCC. |
|
@yxsamliu being able to compile something doesn't mean it is correct, or should be done. HCC compiling that is an error and a hack which was put in place a long time ago and ignores all of the issues I described above. Since we (AMD) own RCCL, we should make sure it doesn't do silly things such as use |
|
I will try the latest tensorflow with rccl and come back to this. |
|
@yxsamliu I don't think that anything will change unless RCCL stops using |
|
It seems the latest rccl does not use std::atomic. I just want to make sure if they already removed those. |
|
This suggests that, unfortunately, it still does: https://github.com/ROCmSoftwarePlatform/rccl/search?q=std%3A%3Aatomic&unscoped_q=std%3A%3Aatomic, but hopefully I am missing something. |
|
I opened issue ROCm/rccl#54 for rccl. Let's see their response. |
|
close this since rccl agrees to replace std::atomics with hip atomics. |
No description provided.