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

Build GPU code only for arch enabled in Kokkos with HIP #652

Closed
wants to merge 2 commits into from

Conversation

dalg24
Copy link
Contributor

@dalg24 dalg24 commented Mar 8, 2022

I realized we were compiling the code for all supported architectures because build time is ridiculous and more particularly I was getting warnings multiple times.
To be honest I am not sure yet what is the best way to do this. For reference see https://github.com/ROCm-Developer-Tools/hipamd/blob/0d7eebeccaa4afd58b8ebddd5c6304e0eef0b422/hip-config.cmake.in#L166 and https://rocmdocs.amd.com/en/latest/Installation_Guide/Using-CMake-with-AMD-ROCm.html#using-hip-in-cmake

@dalg24 dalg24 requested a review from Rombur March 8, 2022 21:17
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@Rombur Rombur left a comment

Choose a reason for hiding this comment

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

Do we always need this code or only when using rocthrust?

CMakeLists.txt Outdated Show resolved Hide resolved
@aprokop aprokop added the build Build and installation label Mar 8, 2022
Copy link
Contributor

@aprokop aprokop left a comment

Choose a reason for hiding this comment

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

I don't understand how this works.

  • Where is GPU_TARGETS used?
  • Why do we need to do this for HIP and not CUDA?
  • Why do we need to do this in ArborX and not inherit it from the built Kokkos?

@dalg24
Copy link
Contributor Author

dalg24 commented Mar 8, 2022

It influences the find_package cascade when we look for rocThrust. If absent, we end up building for all supported architectures.
This is not applicable to CUDA.
Not sure whether Kokkos can nor should do anything about it. Regardless in the meantime we should consider this patch.

@dalg24
Copy link
Contributor Author

dalg24 commented Mar 8, 2022

Do we always need this code or only when using rocthrust?

No you are right, only when using rocThrust

include(CMakeDependentOption)
cmake_dependent_option(ARBORX_ENABLE_ROCTHRUST "Enable rocThrust support" ON "Kokkos_ENABLE_HIP" OFF)
if(Kokkos_ENABLE_HIP AND ARBORX_ENABLE_ROCTHRUST)
set_amd_gpu_target()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rombur let me know what you think about the function.
We would need to put in a separate CMake file that gets installed and call it again in our exported configuration before finding the rocThrust dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @Rombur

@aprokop
Copy link
Contributor

aprokop commented May 31, 2022

What do we need to do to move forward with this PR?

@dalg24
Copy link
Contributor Author

dalg24 commented May 31, 2022

I think for now we should stick with configuring with -DGPU_TARGETS=...
CC @Rombur

@dalg24 dalg24 marked this pull request as draft June 6, 2022 21:07
@aprokop
Copy link
Contributor

aprokop commented Oct 14, 2022

Can we use any of kokkos/kokkos#5327 here?

@Rombur
Copy link
Collaborator

Rombur commented Oct 14, 2022

I don't think so. The fist message in that PR is wrong btw. This is not how hipcc works.

Comment on lines +23 to +34
if(VEGA900 IN_LIST Kokkos_ARCH)
set(Kokkos_AMD_GPU_TARGET gfx900)
elseif(VEGA906 IN_LIST Kokkos_ARCH)
set(Kokkos_AMD_GPU_TARGET gfx906)
elseif(VEGA908 IN_LIST Kokkos_ARCH)
set(Kokkos_AMD_GPU_TARGET gfx908)
elseif(VEGA90A IN_LIST Kokkos_ARCH)
set(Kokkos_AMD_GPU_TARGET gfx90a)
else()
message(WARNING "GPU device code will be built for all architectures suported by ROCm.")
return()
endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer necessary with kokkos/kokkos#5925

@dalg24
Copy link
Contributor Author

dalg24 commented Apr 12, 2024

Closing since this is being addressed in Kokkos from release 4.3
We might want to document somewhere in the build instructions the recommendation to configure with -DGPU_TARGETS=gfx90a until then if we haven't already done so.

@dalg24 dalg24 closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build and installation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants