-
Notifications
You must be signed in to change notification settings - Fork 58
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
papi(bug): use kp_add_library
to ensure it is installed
#222
Conversation
4316860
to
ce606e8
Compare
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.
Maybe we should not get hung up on the name and revisit later if the need arise.
@@ -9,7 +9,8 @@ | |||
"CMAKE_CXX_STANDARD" : "17", | |||
"KokkosTools_ENABLE_EXAMPLES" : "ON", | |||
"KokkosTools_ENABLE_SINGLE" : "ON", | |||
"KokkosTools_ENABLE_MPI" : "ON" | |||
"KokkosTools_ENABLE_MPI" : "ON", | |||
"KokkosTools_ENABLE_PAPI" : "ON" |
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 understand that is the easiest way to get this tested in all builds but I am not sure whether enabling PAPI should be configured as the "default" build. That is the name of the preset that you changed.
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.
At first, I thought it would only make sense to test the PAPI
connector for CPU-related execution spaces.
But according to https://github.com/icl-utk-edu/papi/wiki (thought I haven't read the details), PAPI
claims to also work for GPUs (e.g. https://github.com/icl-utk-edu/papi/wiki/PAPI-Component-ROCM).
I'm not against testing PAPI
only for the presets like OpenMP
. I'll do what you advise 😉
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 did not request changes)
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.
At first, I thought it would only make sense to test the
PAPI
connector for CPU-related execution spaces.But according to https://github.com/icl-utk-edu/papi/wiki (thought I haven't read the details),
PAPI
claims to also work for GPUs (e.g. https://github.com/icl-utk-edu/papi/wiki/PAPI-Component-ROCM).I'm not against testing
PAPI
only for the presets likeOpenMP
. I'll do what you advise 😉
Thanks very much for these changes and thanks for your particular question on PAPI GPU library support.
I absolutely think PAPI should be tested for more than presets like OpenMP.
Given my own prior experience with PAPI, and recent interactions with Kokkos users and developers, Kokkos should allow for PAPI profiling for GPUs and not just CPUs. With support for PAPI in CUDA for more than a few years and recent maturity of PAPI for HIP over the past year, I think it is fundamentally important that Kokkos Tools do tests on PAPI GPU libraries.
Broadly speaking: Kokkos users should not have to rely on vendor's derived metrics through their visualization tools. They should be able to have direct access to hardware counters. I think this true for any scientific application program using CUDA, HIP, SyCL.
I don't know if there is a SYCL PAPI.
With all the above said, I think that In principle, putting this particular change on PAPI GPU libraries in a separate Kokkos Tools PR may make more sense so that your non PAPI GPU related can be merged quickly.
@dalg24 ?
Agreed. |
Also, note that PAPI isn't available everywhere, sometimes even on CPUs (so I think having it as the default Kokkos Tools build may not be desirable). |
The
PAPI
connector was not part of the installed files, because it was added withadd_library
instead ofkp_add_library
.This PR also adds
libpapi-dev
in thebuild-with-kokkos.yaml
pipeline so we can at least build thePAPI
connector.