bpickrel
commented
4 weeks ago
- The time measurements should be done in the
find_matches_forfunction.
@pfultz2 does every single matcher go through find_matches_for? If we're going to add this feature to the develop branch then I suggest you add it for every matcher, not just the selected ones that Aarushi added the code to.
- There should also be an environment variable to enable it like
MIGRAPHX_TIME_MATCHERS.
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?
- Trace filter should probably be used to reduce down the amount of printouts.
I suggest wrapping the timer function in a macro so you can easily insert/remove it from the list in individual passes' find_matches() methods, eg.
void simplify_algebra::apply(module& m) const
{
// Run simplifications multiple times
m.repeat_while_changes(8, [&] {
match::find_matches(m,
LOG_MATCHER_TIMER(find_inner_broadcast{}), <===new macro
find_dot_broadcast{},
etc.which would be less repetitive work than wrapping each matcher in curly brackets as Aarushi has done.
What do you think?
pfultz2
migraphx-bot
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Additional details and impacted files
```diff @@ Coverage Diff @@ ## develop #3500 +/- ## =========================================== + Coverage 92.02% 92.14% +0.12% =========================================== Files 509 512 +3 Lines 21005 21428 +423 =========================================== + Hits 19330 19745 +415 - Misses 1675 1683 +8 ```:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.