-
Notifications
You must be signed in to change notification settings - Fork 83
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
matcher logger #3500
base: develop
Are you sure you want to change the base?
matcher logger #3500
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3500 +/- ##
===========================================
+ Coverage 92.02% 92.16% +0.13%
===========================================
Files 509 512 +3
Lines 21005 21404 +399
===========================================
+ Hits 19330 19727 +397
- Misses 1675 1677 +2 ☔ View full report in Codecov by Sentry. |
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.
- The time measurements should be done in the
find_matches_for
function. - There should also be an environment variable to enable it like
MIGRAPHX_TIME_MATCHERS
. - Trace filter should probably be used to reduce down the amount of printouts.
src/simplify_algebra.cpp
Outdated
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.
Update copyright date (think we have a script for that; can't remember how to run it)
@pfultz2 does every single matcher go through
This is a good way to globally enable/disable match timing. But maybe we also want to enable/disable timing programmatically for individual matchers? Aarushi, in your experience would this be helpful?
I suggest wrapping the timer function in a macro so you can easily insert/remove it from the list in individual passes'
which would be less repetitive work than wrapping each matcher in curly brackets as Aarushi has done. What do you think? |
Yes.
The
I would like to avoid needing to change the source code to get timings. We dont do it for the passes. This will let us get timings on deployed systems so we dont need to rebuild migraphx. |
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
auto r = match_instruction(get_module(mod), ins, m.matcher()); | ||
if(r.result == get_module(mod).end()) | ||
auto elapsed_time = time<std::chrono::nanoseconds>([&] { | ||
auto r = match_instruction(get_module(mod), ins, m.matcher()); |
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.
We should only be timing the match and apply methods. We should also print out the time for each.
No description provided.