-
Notifications
You must be signed in to change notification settings - Fork 407
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
Implement SYCL::print_configuration #3992
Conversation
core/src/SYCL/Kokkos_SYCL.cpp
Outdated
@@ -143,119 +148,117 @@ void SYCL::impl_initialize(SYCL::SYCLDevice d) { | |||
Impl::SYCLInternal::singleton().initialize(d.get_device()); | |||
} | |||
|
|||
std::ostream& SYCL::SYCLDevice::info(std::ostream& os) const { | |||
std::ostream& SYCL::info(std::ostream& os, const sycl::device& 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.
How about naming this impl_sycl_info. I am not insisting on it, but it took me a while to recognize this is a private function and thus its fine to have it as something which isn't part of the execution space concept. Since its private I am not against it, we may actually jsut want to discuss in the next meeting naming practices for private functions - if we want one or not.
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.
THere is one naming thing (for SYCL::info) I am a bit wondering about. But otherwise fine. Do we need this for 3.4.1?
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.
On second thought, isn't it what
kokkos/core/src/SYCL/Kokkos_SYCL.cpp
Lines 298 to 308 in ba33d02
void SYCLSpaceInitializer::print_configuration(std::ostream& msg, | |
const bool /*detail*/) { | |
msg << "Devices:" << std::endl; | |
msg << " KOKKOS_ENABLE_SYCL: "; | |
msg << "yes" << std::endl; | |
msg << "\nRuntime Configuration:" << std::endl; | |
// FIXME_SYCL not implemented | |
std::abort(); | |
// Experimental::SYCL::print_configuration(msg, detail); | |
} |
was intended for?
Yes, this needed for trilinos/Trilinos#9086. I am fine with changing the name to |
@dalg24 I also fixed |
40b2f01
to
c3144bb
Compare
msg << "Devices:" << std::endl; | ||
msg << " KOKKOS_ENABLE_SYCL: "; | ||
msg << "yes" << std::endl; |
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.
This information is duplicated
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 just tried to be consistent with, say, SYCL
:
$ git grep -n "KOKKOS_ENABLE_HIP" | grep '"'
Makefile.kokkos:1168: tmp := $(call kokkos_append_header,"$H""define KOKKOS_ENABLE_HIP_RELOCATABLE_DEVICE_CODE")
core/src/HIP/Kokkos_HIP_Instance.cpp:105: s << "macro KOKKOS_ENABLE_HIP : defined" << '\n';
core/src/HIP/Kokkos_HIP_Space.cpp:496: msg << " KOKKOS_ENABLE_HIP: ";
core/src/HIP/Kokkos_HIP_Space.cpp:500: msg << " KOKKOS_ENABLE_HIP_RELOCATABLE_DEVICE_CODE: ";
and CUDA
:
$ git grep -n "KOKKOS_ENABLE_CUDA" | grep '"' | grep "<<"
core/src/Cuda/Kokkos_Cuda_Instance.cpp:252: s << "macro KOKKOS_ENABLE_CUDA : defined\n";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:885: msg << " KOKKOS_ENABLE_CUDA: yes\n";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:888: msg << " KOKKOS_ENABLE_CUDA_ATOMICS: ";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:896: msg << " KOKKOS_ENABLE_CUDA_LAMBDA: ";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:902: msg << " KOKKOS_ENABLE_CUDA_LDG_INTRINSIC: ";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:908: msg << " KOKKOS_ENABLE_CUDA_RELOCATABLE_DEVICE_CODE: ";
core/src/Cuda/Kokkos_Cuda_Instance.cpp:914: msg << " KOKKOS_ENABLE_CUDA_UVM: ";
What do you want me to do?
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.
My preference would go towards dropping the macro name in *SpaceInitializer::print_configuration
for these backends.
* Implement SYCL::print_configuration * Rename SYCL::info to SYCL::impl_sycl_info * Implement SYCLSpaceInitializer::print_configuration
Trilinos needs this and we haven't actually implemented it yet although we had an
SYCLDevice::info
that would fulfill similar functionality that I am just using. To combine the implementation I pulled it outside ofSYCLDevice
(but still inSYCL
) as a private static member function (since I don't see a reason to actually store anSYCLDevice
as of now).Also, we are not looping over all possible devices but just the one that is used by the current object so I decided to make
print_configuration
non-static (similar toOpenMPTarget
). I remember @crtrott saying that it's fine forOpenMPTarget
but I am happy to discuss if we want something else.