-
Notifications
You must be signed in to change notification settings - Fork 62
Use enable_language(HIP) #702
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
Conversation
host-configs/llnl/toss_4_x86_64_ib_cray/[email protected][email protected]_hip.cmake
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris White <[email protected]>
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.
Thanks, David!
@@ -41,32 +47,7 @@ message(STATUS "ROCM path: ${ROCM_PATH}") | |||
message(STATUS "HIP version: ${hip_VERSION}") | |||
|
|||
# AMDGPU_TARGETS should be defined in the hip-config.cmake that gets "included" via find_package(hip) | |||
# This file is also what hardcodes the --offload-arch flags we're removing here | |||
if(DEFINED AMDGPU_TARGETS) |
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.
It seems like there's no longer a point for having AMDGPU_TARGETS.
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.
Or is it used internally by hip's CMake setup?
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 don't see it in CMake's source, maybe it is in the FindRocm source. I would have expected this to only be used as a backwards compatiblity and maybe just set the CMake variable.
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 could do backwards compatibility but I think removal might be best since we are going to be having breaking BLT changes soon
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 am okay with that as well.
…o feature/enable_language_hip
host-configs/llnl/toss_4_x86_64_ib_cray/[email protected]_hip.cmake
Outdated
Show resolved
Hide resolved
@@ -87,14 +68,10 @@ else() | |||
endif() | |||
|
|||
blt_import_library(NAME blt_hip | |||
COMPILE_FLAGS ${_blt_hip_compile_flags} |
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 just noticed this: _blt_hip_compile_flags is still set above but it is now unused. Is the fix for --rocm-path with crayftn no longer needed? @davidbeckingsale @white238
Inspired by #596 , but doesn't support hipcc