transformations: New test-add-timers-to-top-level-funcs pass #3407
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3407 +/- ##
=======================================
Coverage 90.15% 90.16%
=======================================
Files 455 455
Lines 57429 57467 +38
Branches 5530 5532 +2
=======================================
+ Hits 51775 51815 +40
Misses 4195 4195
+ Partials 1459 1457 -2 ☔ View full report in Codecov by Sentry. |
| assert res == expected_res | ||
| for e in expected_res: | ||
| assert e in res | ||
| assert len(res) < len(all_passes) |
There was a problem hiding this comment.
This test has a dependency on get_all_passes(), calling get_available_pass_list() on its output, which may cause breaks when new passes are added. A better fix than the above would be to test get_all_passes separately, and to call get_available_pass_list() with a fixed list of passes, as it compares the result to a fixed list of expected available passes.
There was a problem hiding this comment.
I disagree with your change, and agree with your sentiment, if you have the bandwidth to fix this test then it would be welcome. In the meantime, could you please update the expected pass list instead?
There was a problem hiding this comment.
I disagree with the method of testing this and the suggested temporary fix.
There was a problem hiding this comment.
I'll just fix the test today, it's been made much easier recently since Alex fixed the underlying infrastructure, thanks for providing a good reason to do it now rather than later.
There was a problem hiding this comment.
Excellent, thanks, I'm rolling back changes to this file in this PR.
| assert res == expected_res | ||
| for e in expected_res: | ||
| assert e in res | ||
| assert len(res) < len(all_passes) |
There was a problem hiding this comment.
I disagree with your change, and agree with your sentiment, if you have the bandwidth to fix this test then it would be welcome. In the meantime, could you please update the expected pass list instead?
| all_funcs = [f for f in op.body.block.ops if isinstance(f, func.FuncOp)] | ||
| func_names = [f.sym_name.data for f in all_funcs] | ||
| if TIMER_START in func_names or TIMER_END in func_names: | ||
| return | ||
|
|
||
| start_func_t = func.FunctionType.from_lists([], [builtin.Float64Type()]) | ||
| end_func_t = func.FunctionType.from_lists( | ||
| [builtin.Float64Type()], [builtin.Float64Type()] | ||
| ) | ||
| start_func = func.FuncOp(TIMER_START, start_func_t, Region([])) | ||
| end_func = func.FuncOp(TIMER_END, end_func_t, Region([])) | ||
|
|
||
| PatternRewriteWalker( | ||
| AddBenchTimersPattern(start_func_t, end_func_t), apply_recursively=False | ||
| ).rewrite_module(op) | ||
|
|
||
| op.body.block.add_ops((start_func, end_func)) |
There was a problem hiding this comment.
I would recommend using SymbolTable APIs to look for functions and insert them:
xdsl/backend/riscv/lowering/convert_memref_to_riscv.py
| if ( | ||
| not (top_level := op.parent_op()) | ||
| or not isinstance(top_level, builtin.ModuleOp) | ||
| or top_level.parent | ||
| ): | ||
| return |
There was a problem hiding this comment.
We currently don't have a use case for this.
There was a problem hiding this comment.
We're looking at annotating top-level functions that are the equivalent of a main function. Should prob add a check that it's not being called. Don't think the SymbolTable API supports this though by any chance?
There was a problem hiding this comment.
Would the name of the function to annotate be worth adding as a pass parameter? If it works with your flow it might be the easiest way to make it safe by default.
There was a problem hiding this comment.
Good suggestion, let me come back to that after the deadline if alright
…ject#3407) Adds `timer_start()` and `timer_end()` calls at the beginning and end of top-level functions, intended for testing and benchmarking purposes. --------- Co-authored-by: n-io <n-io@users.noreply.github.com>
Adds
timer_start()andtimer_end()calls at the beginning and end of top-level functions, intended for testing and benchmarking purposes.