Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Snippets] Add debug caps for dumping snippets parameters #28378
base: master
Are you sure you want to change the base?
[Snippets] Add debug caps for dumping snippets parameters #28378
Changes from all commits
4a75747
e8387b7
06eee03
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 line break, so the output will be displayed as a table
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.
Done
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.
Do we really need to implement this pass as a template? What is the reasoning behind this?
I'm sure that in the absolute majority of use cases we'll be interested in BrgemmCPU.
We can also map onto snippets::op::Brgemm, if we want to support all Brgemm types
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.
Let's try to come up with more descriptive naming. Since this pass inserts some expressions into LIR, it should probably have
insert
in its name (lineinsert_buffers
orinsert_load_store
). Moreover, there is already a very similar passInsertPerfCount
, so we should name this one in a similar way. Smth likeInsertPerfCountVerbose
orInsertPerfCountCsvDump
or smth like that.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.
Are you sure that we'll never want to override this method?
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.
It's not a very good practice to use node name to communicate some information ("_DebugParams"). If we need
PerfCountBegin
with some special properties/functionality, then we should probably create a derived class for this.UPD: But even if we don't want to create a dedicated class for some reason, I still don't get it why we should rename the node, since we already "marked" it by adding some meta information to
rt_info
.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 have 2 concerns with respect to
csv_path
: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 don't see anything specific to a particular
BRGEMM_TYPE
here, so it's another point to consider mapping onto snippets::op::BrgemmThere 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.
It's quite elegant solution to minimize dumping overheads 👍
I think that we should align dumping behavior for default and csv modes.
One way to do it is implement csv-related functionality in a derived class and and use counters like here.
But if you think about it, what you do here is trying to mimic the shared poiner functionality in a way. So Why not to make another step and move the dumping functionality to a separate class altogether. It would be more scalable and convenient, since different types of output can be supported, or different groups of nodes can output to different files (might be convenient to dump MatMul params & say generic counters to separate files). How it might look:
What do you think?
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.
What is the reason for these conditions on
acc_max
andavg_max
? Why can't they be 0?Also "_DebugParams" should not be used like this, as discussed in the transformation file.