-
Notifications
You must be signed in to change notification settings - Fork 59
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
space-time-stack: allow demangled names #225
Conversation
e54e907
to
0d318fd
Compare
bb730c6
to
999a2c2
Compare
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 make sure that the pull request title and description reflect all the changes you make here.
999a2c2
to
7db6e97
Compare
7db6e97
to
98ae12d
Compare
98ae12d
to
b6d665b
Compare
You have to fix the indentation as well. |
44e8fe0
to
e298daf
Compare
e0cb23d
to
da545cd
Compare
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.
Fine with me.
int kernelIndex = find_index(kernelInfo, new_kernel->getName()); | ||
if (!new_kernel->getName().empty()) { | ||
int kernelIndex = | ||
find_index(kernelInfo, new_kernel->getName().c_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.
This is silly, please update find_index and take the trailing argument as a string const ref.
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.
Indeed, and done.
By the way, I think find_index
is useless if the variable kernelInfo
, which is a std::vector
, is changed to being a std::set
whose comparator compares the time (see what compareKernelPerformanceInfo
does).
I think the code would be much easier to read.
If you agree I can make the change.
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.
@dalg24 I guess you'll want to make the change in another PR 😉
83a770d
to
a6ffd65
Compare
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 post a before/after tool output snippet in the description.
a6ffd65
to
9cc8f85
Compare
I added a test for the space time stack tool that checks how kernels appear in the report. So I had to modify slightly the code such that Kokkos Tools can also parse unnamed kernels launched with a work tag. See the changes for more details. |
d30ca08
to
726b55f
Compare
726b55f
to
45b76b2
Compare
@dalg24 @masterleinad Could you please review this one one last time ? 🔍 Thanks! |
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.
Although obviously adding tests in an improvement but you had almost reached consensus and expanding the scope of your PR will delay merging.
|
||
#include "utils/demangle.hpp" | ||
|
||
template <typename execution_space, size_t size = 5> |
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.
Why would you want to template the whole struct?
Get rid of apply()
, move the test to the constructor that you template on ExecutionSpace
. Also there is no point on making the number of iteration a template parameter.
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.
- I think I templated the struct over the execution_space type to check what the tool would output when looking at kernels defined in templated classes. If you think it is unnecessary, I can get rid of the execution_space template.
- For the number of iterations, I think I wanted to have a compile-time range, but I guess it is useless for this test. I'll use a range from 0 to 1 now 😉
@romintomasetti Sorry to not get back sooner - I was away and on vacation + holiday since this time until late last week, and I am still catching up. The idea of demangle.hpp from your initial post and the current code and CMake in your PR makes sense and looks good to me. My top-level comment is whether you want to test this demangling functionality for a couple of other tools, e.g., kernel-logger, simple-kernel-timer? You put this in the common subdirectory, so am I correct that it can be used for those too? Edit: I see that you have used simple-kernel-timer, so that's great! Right now, I think |
45b76b2
to
b738583
Compare
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.
I would prefer to give the functor with a tag a different name. Otherwise, this looks good to me.
b738583
to
2599665
Compare
2599665
to
de5398b
Compare
As promised in #218, this PR extracts the part relevant to demangling names.
common/utils/demangle.hpp
.This is how the report looks like with the changes:
and how it looks without the changes: