-
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
Add kp_functor_size: print parallel functor sizes #242
base: develop
Are you sure you want to change the base?
Conversation
Prints the count, size, and name of functors passed to Kokkos parallel regions.
Corresponding Core PR: kokkos/kokkos#6844 |
@crtrott I discussed this with Carl. Carl is using this for his work. This would also well-serve Kokkos Tools because it is a basis to have a generalized ToolState struct that the Kokkos Tools users can use to obtain profiling information per kernel invocation. This ToolsState came out of a discussion from @jrmadsen in #147. I looked over this and it looks like the implementation is correct. The only question is whether we want to merge this, or if we want to provide a generalized tool callback than one marking the functor size - maybe something like One note to consider (@cwpearson - we didn't discuss this) is that functor size is compile-time information and doesn't change dynamically at runtime. I wonder whether doing this in tool callbacks is the only way. This PR forms the basis for dynamically changing information/properties of a Kokkos kernel. |
@vlkale I changed the name of the callback to mark_kernel_static_info and defined a 512-byte struct, so while this currently only tracks functor size, it could be extended to include other info. |
Great, thanks! |
…UTPUT_CSV_PATH to control output path
@@ -147,6 +147,7 @@ add_subdirectory(debugging/kernel-logger) | |||
|
|||
# Profilers | |||
if(NOT WIN32) | |||
add_subdirectory(profiling/functor-size) |
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.
Please add a test so that:
- your feature is proven to work as intended
- we can better understand how you want to use it
To me, it's still a bit unclear at the moment...
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 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, thanks for the pointer.
Still, I think a test must be added in Kokkos Tools
itself (even if the feature is considered experimental) 😉
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.
@cwpearson I didn't say this in my review of your PR but yes @romintomasetti is right. We have recently been pushing for testing in other Kokkos Tools connectors. I think having a simple test using Google Test would be good. You can see the test from space time stack in Kokkos Tools develop
branch by @romintomasetti. I can help you with this if you need.
@romintomasetti I think @cwpearson has demonstrated the use cases reasonably well. The PR from @masterleinad, see the table of output that @cwpearson has put in.
Perhaps @cwpearson can check if he needs to update the example
directory of Kokkos Tools, but I don't think he needs to. The Kokkos application programmer need not make any code changes to use this particular tools capability.
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.
Prints the count, size, and name of functors passed to Kokkos parallel regions.