Skip to content
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

ROCm 6.0 replaces all __HIP_PLATFORM_HCC__ with __HIP_PLATFORM_AMD__ #1106

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

junliume
Copy link
Collaborator

CMakeLists.txt Outdated
@@ -243,7 +243,7 @@ if( DEFINED CK_OVERRIDE_HIP_VERSION_PATCH )
endif()
message(STATUS "Build with HIP ${HIP_VERSION}")
link_libraries(hip::device)
add_compile_definitions(__HIP_PLATFORM_HCC__=1)
add_compile_definitions(__HIP_PLATFORM_AMD__=1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to find a way to make this backward compatible

@illsilin illsilin merged commit 3ab1838 into develop Dec 19, 2023
21 of 22 checks passed
@illsilin illsilin deleted the fix_hcc branch December 19, 2023 15:46
Comment on lines +246 to +250
if(CK_hip_VERSION VERSION_GREATER_EQUAL 6.0.23494)
add_compile_definitions(__HIP_PLATFORM_AMD__=1)
else()
add_compile_definitions(__HIP_PLATFORM_HCC__=1)
endif()
Copy link

@atamazov atamazov Dec 19, 2023

Choose a reason for hiding this comment

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

@illsilin @junliume I think that offline compiler must add these definitions on its own, these are predefined macros. In MIOpen we have a patch for HIPRTC because at least at the beginning HIPRTC compilation path didn't provide these. I recommend removing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@illsilin could we remove them? Since CK uses offline compiler at the moment. Or this is noop and will not have any effects.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we use those macros in CK, so it makes no difference.

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.

3 participants