-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add CMake HIP language support #266
base: develop
Are you sure you want to change the base?
Conversation
When passed -DROCRAND_CMAKE_HIP_LANGUAGE=ON, rocRAND will be built using CMake's support for the HIP language.
endif() | ||
option(ROCRAND_CMAKE_HIP_LANGUAGE "Use CMake's HIP language support" OFF) | ||
if(ROCRAND_CMAKE_HIP_LANGUAGE) | ||
enable_language(HIP) |
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.
You still need to call find_package(hip)
when using hip language. Libraries like rocRAND need to provide hip::host
in the targets interface because it includes hip's runtime API headers.
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. I was just about to request your review. I haven't yet tested this with downstream libraries.
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.
Ah wait it looks like rocRAND actually needs hip::device as it includes hip's device headers:
https://github.com/ROCmSoftwarePlatform/rocRAND/blob/develop/library/include/rocrand/rocrand.h#L29
We would need two different targets to support using rocRAND with HIP language and with CXX language.
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's not actually clear to me why it includes <hip/hip_runtime.h>
. rocRAND appears to provide a C99 API. Sadly, the documentation is... lacking.
EDIT: Oh, I see. rocrand.h
is a C API but rocrand.hpp
clearly does need <hip/hip_runtime.h>
.
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.
We would need two different targets to support using rocRAND with HIP language and with CXX language.
Maybe we could detect the language with a generator expression? I'm not entirely sure it's possible, but I was thinking something like:
set_property(TARGET roc::rocrand APPEND PROPERTY
INTERFACE_LINK_LIBRARIES "$<$<NOT:$<COMPILE_LANGUAGE:HIP>>:hip::device>"
)
HI! rocRAND developer here, thank you for getting this off the ground. I believe cmake HIP support for rocRAND would simplify many projects that depend on rocRAND. I added a few comments, and will try this out soon. I think I can help answer some questions regarding the project if you need anything. |
string(REGEX MATCH ".mcode\-object\-version" TARGET_ID_SUPPORT ${CXX_OUTPUT}) | ||
endif() | ||
option(ROCRAND_CMAKE_HIP_LANGUAGE "Use CMake's HIP language support" OFF) | ||
if(ROCRAND_CMAKE_HIP_LANGUAGE) |
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.
IMO CUDA and HIP language support should be uniformly handled in rocRAND, so similar to other projects I would rather have an option like ROCRAND_LANGUAGE
(the name is not important) which could be set to either HIP or CUDA.
If this variable is not set then the old behaviour applies based on the C++ compiler set (hipcc or clang -> HIP without cmake language support, gcc or nvcc -> CUDA), if set it overwrites this detection and forces either HIP or CUDA cmake language.
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.
That's an interesting thought. I'll change this behaviour from ROCRAND_CMAKE_HIP_LANGUAGE=ON
to ROCRAND_CMAKE_LANG=HIP
. The default would be ROCRAND_CMAKE_LANG=CXX
, and if somebody wants to implement CMake CUDA language support, it would be ROCRAND_CMAKE_LANG=CUDA
.
if(ROCRAND_CMAKE_HIP_LANGUAGE) | ||
set_source_files_properties(${rocRAND_TEST_SRCS} | ||
PROPERTIES | ||
LANGUAGE HIP | ||
) | ||
endif() |
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 would be better to have these lines together with the lines for CUDA a few lines below.
@@ -29,7 +34,7 @@ foreach(benchmark_src ${rocRAND_BENCHMARK_SRCS}) | |||
else() | |||
target_link_libraries(${BENCHMARK_TARGET} | |||
rocrand | |||
hip::device | |||
$<$<NOT:$<BOOL:ROCRAND_CMAKE_HIP_LANGUAGE>>:hip::device> |
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.
Nit: for benchmarks an if statement is used, but here its a generator expression. I would stick to the if in both places for consistency, and because this setting is known at configure time, a generator expression is not required.
This has been sitting around for a while. Should it be resurrected? |
Maybe, but I don't have the bandwidth to do that quite yet. The official Linux and Windows builds now both use CMake 3.22, so if this PR were finished, we could begin to use the CMake HIP language support in our official builds. |
Build Instructions
rocm_agent_enumerator
works)With these changes, the above command will build a version of the library compatible with the system it's running on. If you want specific GPU architectures, append
-DCMAKE_HIP_ARCHITECTURES='gfx906:xnack-;gfx1030'
(replacing the examplegfx906:xnack-;gfx1030
values with your chosen semi-colon delimited list of AMDGPU target ids).Related CMake Documentation