-
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
Remove KOKKOS_ENABLE_DEBUG_PRINT_KERNEL_NAMES #4150
Remove KOKKOS_ENABLE_DEBUG_PRINT_KERNEL_NAMES #4150
Conversation
Retest this please. |
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.
Drop KOKKOS_ENABLE_DEBUG_PRINT_KERNEL_NAMES
and the code that it was guarding
Hmm... I want to at least discuss if we really want to drop this feature. People might be surprised that it just disappeared. Of course, deprecating is not really possible either. |
Meeting decision: Drop this entirely: SF F N A SA We drop this without warning and consider it a bug that it even existed (completely untested). Use kernel logger or so, |
3cfd866
to
b23baad
Compare
I changed the pull request to remove KOKKOS_DEBUG_PRINT_KERNEL_NAMES instead. |
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.
What about the void(str)?
Kokkos::fence(); | ||
std::cout << "KOKKOS_DEBUG End parallel_for kernel: " << str << std::endl; | ||
#endif | ||
(void)str; |
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.
do we still need the (void)str?
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.
AFAICT, str
is still used in all cases, so we don't have to do this (and none of the CI checks complains).
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.
ok
Related to #3966 (comment). I think it's a good idea to have a useful preprocessor switch defined through CMake (and thus documented). Of course, the same effect can be achieved by using
Tools
.If we say that that's the proper way and should always be preferred, I would rather remove the functionality in
Core
.edit: We remove KOKKOS_ENABLE_DEBUG_PRINT_KERNEL_NAMES instead.