tool(space-time-stack): demangle name
Summary
This PR ensures that the tool space-time-stack demangles kernel names.
Side quests, questions and related issues
- [ ] I created new files to store what's common across different tools. Tell me what you think @vlkale @dalg24 @masterleinad and how this should be done in your opinion. (
SpaceHandle.hpp,utils_demangle.hppand so on) I think this touches upon the more general problem of duplication in the tools and how this should be tackled. - [x] ~~I changed
Kokkos_Tools_OptimzationGoaltoKokkos_Tools_OptimizationGoalbecause I think it's a typo. I guess @dalg24 will agree that this deserves a separate PR. Note that the same change will be needed inKokkos.~~ Discussion moved to #221. - [ ] I started targeting #191. I guess I'll need Google Test to do so, as mentioned in the issue by @vlkale. I suggest that we add it as a TPL through a submodule approach.
- [ ] For easy testing, I suggest that things like
struct Stackdefined inkp_space_time_stack.cpphas a declaration in a corresponding header file, so tests can directly go ask such structures what they captured during a test. Tell me what you think. - [ ] I was really surprised that the
SpaceHandlerecognizes the space type by looking at the first letters of itsnameattribute. Is there any reason why it's done like this and not via an enumeration? To me, this seems inefficient, because in e.g.Kokkosyou'll need to create theSpaceHandleby creating a string with the space name, and pass it to the tools, that will need to parse that string. This seems to have more impact on the runtime that simply passing anenum... Moreover, several tools define their own enumeration for a subset of availableKokkosspaces. I would propose that by default theSpaceHandleonly has an attributetypethat comes from an enumeration like the one I defined inSpaceHandle.hpp. What do you think? - [ ] I created a new
enum FrameTypethat seeks to homogenize how kernel types are treated amongst tools, and how they are printed. This also deserves another PR if you agree on the principle.
@maartenarnst Thanks for this earlier. This PR is helpful. Sorry for the delayed reply due to conferences and US holidays this time of year.
Summary
This PR ensures that the tool
space-time-stackdemangles kernel names.Side quests, questions and related issues
- [ ] I created new files to store what's common across different tools. Tell me what you think @vlkale @dalg24 @masterleinad and how this should be done in your opinion. (
SpaceHandle.hpp,utils_demangle.hppand so on) I think this touches upon the more general problem of duplication in the tools and how this should be tackled.
Yes, the issue of duplication across tools will definitely make development of tool connector much easier for someone new to Kokkos Tools. Perhaps it is OK
- [ ] I changed
Kokkos_Tools_OptimzationGoaltoKokkos_Tools_OptimizationGoalbecause I think it's a typo. I guess @dalg24 will agree that this deserves a separate PR. Note that the same change will be needed inKokkos.
Thanks for that! I don't know how much this is being used and how much it impacts other Kokkos Tools from a quick look. Yes, let me know if you can easily submit a new PR on your end, and we will look at it to get this quick fix merged.
- [ ] I started targeting Create unit tests for all Kokkos Tools connectors and utilities #191. I guess I'll need Google Test to do so, as mentioned in the issue by @vlkale. I suggest that we add it as a TPL through a submodule approach.
Yes, I think Google Test was a generally accepted way to do testing of Kokkos Tools when I wrote that Ghub Issue, having discussed with @crtrott previously months ago. Also, Google Tests is used in Kokkos Core. If you know of another dominant and here-to-stay framework for unit tests, let us know, but my opinion from experience with GTests is that it is straightforward and reasonably documented and still the best way to go for now.
- [ ] For easy testing, I suggest that things like
struct Stackdefined inkp_space_time_stack.cpphas a declaration in a corresponding header file, so tests can directly go ask such structures what they captured during a test. Tell me what you think.
I think that this is a good idea. I don't see any issues with putting struct Stack in the header file. What other parts of kp_space_time_stack.cpp were you thinking to move?
- [ ] I was really surprised that the
SpaceHandlerecognizes the space type by looking at the first letters of itsnameattribute. Is there any reason why it's done like this and not via an enumeration? To me, this seems inefficient, because in e.g.Kokkosyou'll need to create theSpaceHandleby creating a string with the space name, and pass it to the tools, that will need to parse that string. This seems to have more impact on the runtime that simply passing anenum... Moreover, several tools define their own enumeration for a subset of availableKokkosspaces. I would propose that by default theSpaceHandleonly has an attributetypethat comes from an enumeration like the one I defined inSpaceHandle.hpp. What do you think?
Deferring to @dalg24, @crtrott and maybe @masterleinad. Your points seem reasonable and good to me but we probably ought to think whether what you suggest is the only/best solution.
- [ ] I created a new
enum FrameTypethat seeks to homogenize how kernel types are treated amongst tools, and how they are printed. This also deserves another PR if you agree on the principle.
Deferring to @dalg24, @crtrott. I like the motivation of the problem. We probably need more discussion on the solution to the problem.
For the last two bullet points, I suggest putting up a GitHub Issue for each first in order to identify the right solutions for them.
@vlkale I've made #234 for discussing about the enum FrameType.
I have already mentioned this to @romintomasetti through DM but I am passing on here that there is a problem with Google Test relying on std::cout. A large number of Kokkos Tools connectors use printf's and not std::cout, including kernel-logger.
@romintomasetti I am thinking we will probably want to go with the Python script post-processing solution right now, and let all tools connectors continue to use printf's.