-
Notifications
You must be signed in to change notification settings - Fork 76
Initial changes for HIP-Clang Compiler #23
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,12 +42,20 @@ target_include_directories(rocprim | |
| # This target allows using only HC interface, links only | ||
| # against HC/HSA library, doesn't require HIP | ||
| add_library(rocprim_hc INTERFACE) | ||
| if(HIP_COMPILER STREQUAL "HCC") | ||
| target_link_libraries(rocprim_hc | ||
| INTERFACE | ||
| rocprim | ||
| hcc::hccrt | ||
| hcc::hc_am | ||
| ) | ||
| elseif(HIP_COMPILER STREQUAL "clang") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it mean that clang also supports HC C++ API?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HIP-Clang shouldn't support HC C++ API. Does that mean I should remove rocprim_hc entirely for HIP-Clang?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I can check what's the best way to do that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can safely disable it for for HIP-Clang. You disabled HC tests/benchmarks/examples anyway, I think the only other place |
||
| target_link_libraries(rocprim_hc | ||
| INTERFACE | ||
| rocprim | ||
| ) | ||
| endif() | ||
|
|
||
| target_compile_definitions(rocprim_hc | ||
| INTERFACE | ||
| ROCPRIM_HC_API=1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ namespace detail | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::atomic_fetch_add(address, value); | ||
| #else | ||
| return ::atomicAdd(address, value); | ||
| return atomicAdd(address, value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why making sure we're calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought the atomic functions in HIP were static, but I notice that isn't true
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is that relevant? |
||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -43,7 +43,7 @@ namespace detail | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::atomic_fetch_add(address, value); | ||
| #else | ||
| return ::atomicAdd(address, value); | ||
| return atomicAdd(address, value); | ||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -53,7 +53,7 @@ namespace detail | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::atomic_fetch_add(address, value); | ||
| #else | ||
| return ::atomicAdd(address, value); | ||
| return atomicAdd(address, value); | ||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -63,7 +63,7 @@ namespace detail | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::atomic_fetch_add(reinterpret_cast<uint64_t*>(address), static_cast<uint64_t>(value)); | ||
| #else | ||
| return ::atomicAdd(address, value); | ||
| return atomicAdd(address, value); | ||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -73,7 +73,7 @@ namespace detail | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::__atomic_wrapinc(address, value); | ||
| #else | ||
| return ::atomicInc(address, value); | ||
| return atomicInc(address, value); | ||
| #endif | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,8 +75,7 @@ unsigned int lane_id() | |
| #ifdef ROCPRIM_HC_API | ||
| return hc::__lane_id(); | ||
| #else // HIP | ||
| // TODO: Find HIP function for that | ||
| return hc::__lane_id(); | ||
| return __lane_id(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does HIP in ROCm 1.8.2 have this function? If not then I am not sure if can merge it to Actually we have branch on our private repo which replaces HC intrinsics with HIP functions (in some places intrinsics just weren't in HIP, in some they didn't work).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function seems to exist on HIP branch roc-1.8.x, I'm not sure if that is specifically ROCm 1.8.2. Do you remember which intrinsic were missing or didn't work?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does rocPRIM have its own rocm 1.8.x branch which can be used on ROCm 1.8.2, and develop used on the master of HIP?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but we can do it the other way around: have a special branch for changes required for hip-clang and HIP master. |
||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -262,19 +261,19 @@ namespace detail | |
| ROCPRIM_DEVICE inline | ||
| void memory_fence_system(void) | ||
| { | ||
| ::__threadfence_system(); | ||
| __threadfence_system(); | ||
| } | ||
|
|
||
| ROCPRIM_DEVICE inline | ||
| void memory_fence_block(void) | ||
| { | ||
| ::__threadfence_block(); | ||
| __threadfence_block(); | ||
| } | ||
|
|
||
| ROCPRIM_DEVICE inline | ||
| void memory_fence_device(void) | ||
| { | ||
| ::__threadfence(); | ||
| __threadfence(); | ||
| } | ||
| #else | ||
| // __threadfence_system() | ||
|
|
||
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.
__HIP_PLATFORM_HCC__is also defined on clang with HIP backend?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.
Seems like it, I ran into an error for hc::__bitextract_u64 without this change
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.
Those are HSA functions. In HIP-Clang they are just in global namespace?
Uh oh!
There was an error while loading. Please reload this page.
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.
Does
__bitextract_u64work when usinghcc? Because it does not work on ROCm 1.8.2.Uh oh!
There was an error while loading. Please reload this page.
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.
I think we can do this:
or
In 2nd option hipCUB (HIP code) won't depend on
hc::even when compiled withhcc. I guess hip-clang does not define__HCC__.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.
Good point, thanks I will change that