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

Introduce KOKKOS_IMPL_ARCH_NVIDIA_GPU macro #5948

Merged
merged 7 commits into from Mar 23, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Mar 2, 2023

We already have KOKKOS_ARCH_INTEL_GPU. It is useful for backends like SYCL, OpenMPTarget, or OpenACC, that may target GPUs from different vendors.
I am not sure whether we want it to be a public or a private macro. We could consider defining it to be the compute capability as an integer but that means more complicate logic in the generated makefiles and in CMake.

Was proposed at last developer meeting when discussing #5867

@dalg24 dalg24 added Refactor Tidying up: make code code, cleaner, simpler, understandable and robust Backend - CUDA labels Mar 2, 2023
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@masterleinad
Copy link
Contributor

Not quite sure why GitHub CI is not running.

@dalg24 dalg24 force-pushed the kokkos_arch_nvidia_gpu_macro branch from c9fc6de to 4dcb294 Compare March 3, 2023 12:13
@dalg24
Copy link
Member Author

dalg24 commented Mar 3, 2023

Not quite sure why GitHub CI is not running.

Retriggering CI. Didn't change anything, just amended nothing to the last commit.

@masterleinad
Copy link
Contributor

masterleinad commented Mar 6, 2023

Only one of the HIP CI runs is failing with the usual errors.

@ajpowelsnl ajpowelsnl requested a review from crtrott March 6, 2023 21:01
@crtrott
Copy link
Member

crtrott commented Mar 7, 2023

I think we really should consider making this thing have the CC as a value, but not blocking for now.

@dalg24
Copy link
Member Author

dalg24 commented Mar 8, 2023

I think we really should consider making this thing have the CC as a value, but not blocking for now.

I implemented your suggestion. Setting the macro in the code rather than with CMake as you wanted.

@dalg24 dalg24 force-pushed the kokkos_arch_nvidia_gpu_macro branch from 84c7ca8 to f670cae Compare March 8, 2023 13:41
@PhilMiller
Copy link
Member

Should we initially introduce this as KOKKOS_IMPL_ARCH_NVIDIA_GPU to not commit to exposing it?

@dalg24
Copy link
Member Author

dalg24 commented Mar 8, 2023

Should we initially introduce this as KOKKOS_IMPL_ARCH_NVIDIA_GPU to not commit to exposing it?

I brought this up at the developer meeting last week and it did not seem to be a concern. I do not feel strongly either way.

@PhilMiller
Copy link
Member

Sorry, I missed that. It's fine by me as is. I just wanted to make sure it was considered

Copy link
Member Author

@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.

Per dev meeting, make it an IMPL macro

@dalg24 dalg24 changed the title Introduce KOKKOS_ARCH_NVIDIA_GPU macro Introduce KOKKOS_IMPL_ARCH_NVIDIA_GPU macro Mar 8, 2023
@dalg24
Copy link
Member Author

dalg24 commented Mar 9, 2023

Retest this please

@dalg24 dalg24 marked this pull request as draft March 9, 2023 16:17
@dalg24
Copy link
Member Author

dalg24 commented Mar 18, 2023

Clang+CUDA build failed with

5: [ RUN      ] cuda_DeathTest.view_memory_access_violations_from_device
5: /var/jenkins/workspace/Kokkos/core/unit_test/TestViewMemoryAccessViolation.hpp:50: Failure
5: Death test: { Kokkos::parallel_for(Kokkos::RangePolicy<ExecutionSpace>(s, 0, 1), *this); Kokkos::fence(); }
5:     Result: died but not with expected error.
5:   Expected: contains regular expression "Kokkos::View ERROR: attempt to access inaccessible memory space.*UNAVAILABLE"
5: Actual msg:
5: [  DEATH   ] cudaDeviceSynchronize() error( cudaErrorAssert): device-side assert triggered /var/jenkins/workspace/Kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:135
5: [  DEATH   ] Backtrace:
5: [  DEATH   ]                        [0x1220b90]
5: [  DEATH   ]                        [0x1215178]
5: [  DEATH   ]                        [0x12151f8]
5: [  DEATH   ]                        [0x1228c8f]
5: [  DEATH   ]                        [0x1228593]
5: [  DEATH   ]                        [0x11fc97d]
5: [  DEATH   ]                         [0x982e5b]
5: [  DEATH   ]                         [0x982481]
5: [  DEATH   ]                         [0x973c1d]
5: [  DEATH   ]                        [0x11ea1a4]
5: [  DEATH   ]                        [0x11c2238]
5: [  DEATH   ]                        [0x11c35d0]
5: [  DEATH   ]                        [0x11c3e9b]
5: [  DEATH   ]                        [0x11d3cce]
5: [  DEATH   ]                        [0x11ead74]
5: [  DEATH   ]                        [0x11d36ec]
5: [  DEATH   ]                         [0x416980]
5: [  DEATH   ] __libc_start_main [0x7f588b3f3bf7]
5: [  DEATH   ]                         [0x41688a]
5: [  DEATH   ] 
5: [  FAILED  ] cuda_DeathTest.view_memory_access_violations_from_device (11253 ms)
5: [----------] 7 tests from cuda_DeathTest (123097 ms total)

Looks unrelated but rerunning to be safe.
Retest this please.

@dalg24 dalg24 marked this pull request as ready for review March 22, 2023 19:06
@dalg24 dalg24 requested a review from PhilMiller March 22, 2023 19:07
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me (apart from Bruno's questions).

Co-authored-by: Phil Miller <unmobile+gh@gmail.com>
@dalg24 dalg24 merged commit 3fc7789 into kokkos:develop Mar 23, 2023
25 checks passed
@dalg24 dalg24 deleted the kokkos_arch_nvidia_gpu_macro branch March 23, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend - CUDA Refactor Tidying up: make code code, cleaner, simpler, understandable and robust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants