This patch is separated from https://reviews.llvm.org/D45212.
Patch by Greg Rodgers.
Revised and lit tests added by Yaxun Liu.
Details
Details
- Reviewers
rjmccall tra - Commits
- rG8391387f7b39: [HIP] define __CUDA_ARCH_=1 for amdgcn targets
rG8a5fc15aa45a: [CUDA] Add amdgpu sub archs
rL329420: [HIP] define __CUDA_ARCH_=1 for amdgcn targets
rC329420: [HIP] define __CUDA_ARCH_=1 for amdgcn targets
rL329232: [CUDA] Add amdgpu sub archs
rC329232: [CUDA] Add amdgpu sub archs
Diff Detail
Diff Detail
- Repository
- rC Clang
Event Timeline
Comment Actions
I didn't get a chance to review the patch before it got committed.
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | Unless you're planning to guarantee 1:1 match to functionality provided by nvidia's sm_32, it would be prudent to use some other value for the macro so the source code has a way to tell these GPUs apart. Another issue with this approach is that typical use pattern for CUDA_ARCH is | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | I think let __CUDA_ARCH__==1 for amdgcn is reasonable and I can make that change. On the other hand, I think it may be difficult to define __HIP_ARCH__ which can sort mixed nvptx/amdgcn GPU's by capability. I do think a well defined __HIP_ARCH__ would be useful for users. Just need some further discussion how to define it. For now, if there are specific codes for nvptx, it can continue use __CUDA_ARCH__. If there are specific codes for amdgcn, it can check predefined amdgpu canonical names, e.g. __gfx803__, etc. | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | OK. | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | I asked Ben Sander about whether we can define HIP_ARCH, here is his answer: HIP targets a broader set of hardware than just a single vendor so additional flexibility in defining feature capability is required. The HIP_ARCH_ macro provide per-feature-granularity mechanism to query features. Also the code tends to be more clear as opposed to an "if __CUDA_ARCH>3 ..assume some feature". For example // 32-bit Atomics: #define __HIP_ARCH_HAS_GLOBAL_INT32_ATOMICS__ (1) #define __HIP_ARCH_HAS_GLOBAL_FLOAT_ATOMIC_EXCH__ (1) #define __HIP_ARCH_HAS_SHARED_INT32_ATOMICS__ (1) #define __HIP_ARCH_HAS_SHARED_FLOAT_ATOMIC_EXCH__ (1) #define __HIP_ARCH_HAS_FLOAT_ATOMIC_ADD__ (1) // 64-bit Atomics: #define __HIP_ARCH_HAS_GLOBAL_INT64_ATOMICS__ (1) #define __HIP_ARCH_HAS_SHARED_INT64_ATOMICS__ (0) // Doubles #define __HIP_ARCH_HAS_DOUBLES__ (1) // warp cross-lane operations: #define __HIP_ARCH_HAS_WARP_VOTE__ (1) #define __HIP_ARCH_HAS_WARP_BALLOT__ (1) #define __HIP_ARCH_HAS_WARP_SHUFFLE__ (1) #define __HIP_ARCH_HAS_WARP_FUNNEL_SHIFT__ (0) // sync #define __HIP_ARCH_HAS_THREAD_FENCE_SYSTEM__ (1) #define __HIP_ARCH_HAS_SYNC_THREAD_EXT__ (0) // misc #define __HIP_ARCH_HAS_SURFACE_FUNCS__ (0) #define __HIP_ARCH_HAS_3DGRID__ (1) #define __HIP_ARCH_HAS_DYNAMIC_PARALLEL__ (0) | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | I assume that will be handled somewhere else -- different patch, different place. While we're looking a this, is CUDA compatibility one of your goals? I.e. do you expect existing CUDA code to be compilable and functional on AMD hardware? If not, then, perhaps you don't need __CUDA_*__ defines at all. | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | CUDA code needs to be translated to HIP code since the host API is different. In most cases the translation can be done by script automatically. __CUDA_ARCH__ cannot be automatically translated because it is not portable to non-nvptx devices, however it is often used to indicate device compilation. Therefore we still need to define it in HIP to indicate device compilation. In this way, CUDA programs using __CUDA_ARCH__ just for checking device compilation can be automatically translated. If users want to use features associated with specific __CUDA_ARCH__ they can manually modify the translated code to use __HIP_ARCH_HAS_* macros. | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | It sounds like this translation is a one-time offline process and HIP-mode compiler is not going to see any non-HIP code. If that's the case, I'm not quite sure I see the need for defining __CUDA_ARCH__ in HIP mode -- translation process should've converted the CUDA-specific macro in the original code to it's HIP equivalent or get user to part it to something HIP can deal with. HIP programming guide also says that __CUDA_ARCH__ is undefined by hcc. | |
| lib/Basic/Targets.cpp | ||
|---|---|---|
| 161 | Sorry I missed that. I will revert the change about macro __CUDA_ARCH__ and define __HIP_DEVICE_COMPILE__ instead. Thanks. | |
Unless you're planning to guarantee 1:1 match to functionality provided by nvidia's sm_32, it would be prudent to use some other value for the macro so the source code has a way to tell these GPUs apart.
Another issue with this approach is that typical use pattern for CUDA_ARCH is
#if __CUDA_ARCH__ >= XXX. I don't expect that we'll always be able to maintain order across GPU architectures among NVIDIA and AMD GPUs. Perhaps for HIP compilation it would make more sense to define CUDA_ARCH as 1 (this should serve as a legacy indication of device-side compilation) and define HIP_ARCH to indicate which AMD GPU we're compiling for without accidentally enabling something that was intended for NVIDIA's GPUs only.