Use eventset in roctxconnector (as in nvtxconnector) #219
Conversation
|
@maartenarnst @romintomasetti Thanks very much for this. I plan to review this later this week or early next. From looking at your PR code changes, it makes sense to me and would be useful change. |
| target_include_directories(kp_roctx_connector PRIVATE ${ROCM_ROCTX_INCLUDE}) | ||
| target_link_libraries(kp_roctx_connector PRIVATE ${ROCM_ROCTX_LIB}) | ||
| target_include_directories(kp_roctx_connector PUBLIC ${ROCM_ROCTX_INCLUDE}) | ||
| target_link_libraries(kp_roctx_connector PUBLIC ${ROCM_ROCTX_LIB}) |
There was a problem hiding this comment.
Why did you change?
I can see how the linking would need some symbols to be private but not why the include directories would be needed downstream.
There was a problem hiding this comment.
These are leftovers from when I was trying to make the roctx-connector work with rocprof. I've reverted the change.
In fact, in my first tries, I used rocprof as follows:
rocprof --hip-trace --hsa-trace --roctx-trace ./a.out --kokkos-tools-libs=${KOKKOS_TOOLS_DIR}/lib/libkp_roctx_connector.so
That didn't work, in the sense that even though this command asks rocprof to use --roctx-trace, rocprof didn't include the markers in the results.json trace. I suspect that the reason may be that rocprof wants to see the executable linked to roctx64.so. That may not be the case when the connector is linked at runtime via dlsym. And so these leftovers came from attempts to make roctx64.so appear among the linked libraries.
| memset(&my_event_set, 0, | ||
| sizeof(my_event_set)); // zero any pointers not set here |
There was a problem hiding this comment.
Uggg. That thing really something that should be handled at construction.
I suspect value-initialization
EventSet my_event_set{};does what you want.
There was a problem hiding this comment.
Indeed :) I used this piece of code because all connectors are currently doing it this way. It may be best to address the issue for all connectors in a separate PR? One way forward might be to provide default initialisers using = NULL for the function pointers in the declaration of the struct in Kokkos_Profiling_C_interface.h?
There was a problem hiding this comment.
I used this piece of code because all connectors are currently doing it this way.
You are right. This is fine then.
And yes definitely fix elsewhere.
There was a problem hiding this comment.
OK. I made issue
dalg24
dismissed
their stale review
Not formatted properly (?) CI log is somewhat useless...
|
Hi @dalg24. Indeed, I've just improved the formatting. It seems the workflows need an approval to run. Would you have a moment to give an approval to see if it passes the formatting test now? Thanks. |
@dalg24 cc @masterleinad thanks |
This PR
roctxconnectorby defining all the functions with the event set and the helper macros, like what is done innvtxconnector. One of the interests of this approach is that it allows the callbacks to be set in a Kokkos program withget_event_set("roctx-connector",...)and thenset_callbacks.On the side, this PR also
namespace KokkosTools::ROCTXConnectorkokkosp_request_tool_settingssuch that fencing can be controlled withKOKKOS_TOOLS_GLOBALFENCESNote that I've also tried to update the
CMakeLists.txtfor the roctxconnector to see if something could be done with a CMake target for roctx/roctracer, but I wasn't able to find such targets in the ROCm directories.This is joint work with @romintomasetti.
@vlkale, @crtrott, would you have a moment to take a look, or refer to someone to take a look? Thanks in advance!