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.

Sign up for GitHub

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

Jump to bottom

[HIP] Use hipcc architecture autodetection when Kokkos_ARCH is not set #3941

Merged
merged 1 commit into from
Apr 29, 2021
Merged

[HIP] Use hipcc architecture autodetection when Kokkos_ARCH is not set #3941

merged 1 commit into from
Apr 29, 2021

Conversation

Rombur
Copy link
Member

@Rombur Rombur commented Apr 14, 2021

If no architecture is selected hipcc does the right thing.

Closes #3933

@masterleinad
Copy link
Contributor

I remember us running into some difficulties with cross-compiling if no architecture was specified. Does that work now?

@Rombur
Copy link
Member Author

Rombur commented Apr 14, 2021

What do you mean? If there are no GPU on the node you are configuring, there is no way to detect the GPU architecture. That hasn't changed.

@masterleinad
Copy link
Contributor

What do you mean? If there are no GPU on the node you are configuring, there is no way to detect the GPU architecture. That hasn't changed.

I just checked that I don't get an error message with this patch if I don't provide an architecture and compile on a node without GPU. Trying to run on a GPU node then fails.
I very much prefer if we error out in that case instead of letting the users deal with

"hipErrorNoBinaryForGpu: Unable to find code object for all current devices!"

at run time. For auto-detection, I would rather follow the approach we are using for Cuda.

dalg24
dalg24 requested changes Apr 15, 2021
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

IF(KOKKOS_COMPILER_ID STREQUAL HIPCC)
  # CODE YOU ADDED
ELSE()
  MESSAGE(SEND_ERROR ...)
ENDIF()

cmake/kokkos_arch.cmake Outdated
@@ -433,8 +433,14 @@ CHECK_AMDGPU_ARCH(VEGA906 gfx906) # Radeon Instinct MI50 and MI60
CHECK_AMDGPU_ARCH(VEGA908 gfx908)

IF(KOKKOS_ENABLE_HIP AND NOT AMDGPU_ARCH_ALREADY_SPECIFIED)
MESSAGE(SEND_ERROR "HIP enabled but no AMD GPU architecture currently enabled. "
"Please enable one AMD GPU architecture via -DKokkos_ARCH_{..}=ON'.")
FIND_PATH(ROCM_ENUMERATOR_PATH rocm_agent_enumerator)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you chose this over find_program

Copy link
Member Author

Choose a reason for hiding this comment

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

No good reason

dalg24
dalg24 approved these changes Apr 29, 2021
View reviewed changes
cmake/kokkos_arch.cmake
Comment on lines +442 to +443
MESSAGE(SEND_ERROR "HIP enabled but no AMD GPU architecture currently enabled. "
"Please enable one AMD GPU architecture via -DKokkos_ARCH_{..}=ON'.")
Copy link
Member

Choose a reason for hiding this comment

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

Error message could be improved and indicate that auto detection failed

@dalg24 dalg24 merged commit 8a93e9b into kokkos:develop Apr 29, 2021
@Rombur Rombur deleted the hip_autodetect branch May 13, 2021 19:31
@dalg24 dalg24 mentioned this pull request Aug 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

@masterleinad masterleinad masterleinad approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Rombur @masterleinad @dalg24 @crtrott