Skip to content
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

tool(space-time-stack): demangle name #218

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

romintomasetti
Copy link
Contributor

@romintomasetti romintomasetti commented Nov 6, 2023

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.hpp and so on) I think this touches upon the more general problem of duplication in the tools and how this should be tackled.
  • I changed Kokkos_Tools_OptimzationGoal to Kokkos_Tools_OptimizationGoal because 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 in Kokkos. Discussion moved to type: renaming Kokkos_Tools_OptimizationGoal #221.
  • I started targeting Create unit tests for all Kokkos Tools libraries 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.
  • For easy testing, I suggest that things like struct Stack defined in kp_space_time_stack.cpp has 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 SpaceHandle recognizes the space type by looking at the first letters of its name attribute. Is there any reason why it's done like this and not via an enumeration? To me, this seems inefficient, because in e.g. Kokkos you'll need to create the SpaceHandle by 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 an enum... Moreover, several tools define their own enumeration for a subset of available Kokkos spaces. I would propose that by default the SpaceHandle only has an attribute type that comes from an enumeration like the one I defined in SpaceHandle.hpp. What do you think?
  • I created a new enum FrameType that 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.

@romintomasetti romintomasetti force-pushed the space-time-stack-demangle branch 2 times, most recently from 1a76dca to 2d757cd Compare November 6, 2023 10:12
@vlkale vlkale requested a review from crtrott November 30, 2023 20:16
@vlkale
Copy link
Contributor

vlkale commented Nov 30, 2023

@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-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.hpp and 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_OptimzationGoal to Kokkos_Tools_OptimizationGoal because 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 in Kokkos.

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.

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 Stack defined in kp_space_time_stack.cpp has 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 SpaceHandle recognizes the space type by looking at the first letters of its name attribute. Is there any reason why it's done like this and not via an enumeration? To me, this seems inefficient, because in e.g. Kokkos you'll need to create the SpaceHandle by 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 an enum... Moreover, several tools define their own enumeration for a subset of available Kokkos spaces. I would propose that by default the SpaceHandle only has an attribute type that comes from an enumeration like the one I defined in SpaceHandle.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 FrameType that 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.

@romintomasetti
Copy link
Contributor Author

@vlkale I've made #234 for discussing about the enum FrameType.

@vlkale
Copy link
Contributor

vlkale commented Mar 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants