-
Notifications
You must be signed in to change notification settings - Fork 236
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
Add initial benchmark setup #1591
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1591 +/- ##
=======================================
Coverage 60.90% 60.90%
=======================================
Files 620 620
Lines 14541 14541
=======================================
Hits 8856 8856
Misses 5685 5685
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Reviewed 3 of 23 files at r1, 17 of 20 files at r2, all commit messages.
Reviewable status: 20 of 23 files reviewed, 3 unresolved discussions (waiting on @chenzhuofu)
lib/compiler/benchmark/src/compiler/series_parallel/computation_graph/get_computation_graph_series_parallel_decomposition.cc
line 13 at r2 (raw file):
static void benchmark_get_computation_graph_series_parallel_decomposition( benchmark::State &state, ComputationGraph const &cg) { // ComputationGraph cg = state.range(0);
remove
lib/compiler/benchmark/src/compiler/series_parallel/computation_graph/get_computation_graph_series_parallel_decomposition.cc
line 30 at r2 (raw file):
get_transformer_computation_graph(get_default_transformer_config())); BENCHMARK_CAPTURE(benchmark_get_computation_graph_series_parallel_decomposition,
is there a slightly better syntax for benchmarking across a set of different values? This isn't bad obviously.
lib/utils/include/utils/archetypes/ordered_value_type.h
line 14 at r2 (raw file):
ordered_value_type(ordered_value_type const &) { PANIC();
the libassert
asserts seem to be a lot nicer trace-wise compared to the cassert
ones, maybe worth it to move all assert logic to libassert
? We could also use -DLIBASSERT_LOWERCASE
to alias assert
to libassert
s assert. (Obviously for a future PR).
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.
Reviewable status: 20 of 23 files reviewed, 3 unresolved discussions (waiting on @chenzhuofu and @Marsella8)
lib/compiler/benchmark/src/compiler/series_parallel/computation_graph/get_computation_graph_series_parallel_decomposition.cc
line 13 at r2 (raw file):
Previously, Marsella8 wrote…
remove
Done.
lib/compiler/benchmark/src/compiler/series_parallel/computation_graph/get_computation_graph_series_parallel_decomposition.cc
line 30 at r2 (raw file):
Previously, Marsella8 wrote…
is there a slightly better syntax for benchmarking across a set of different values? This isn't bad obviously.
You can do numerical ranges (e.g., see transitive_closure.cc
), but for arbitrary sets of values, no. I also wish there was a better method for this, but for now gtest seemed to have the best compromise in terms of features--I'm not against moving to something different later once we have a better idea of how we use the benchmarks
lib/utils/include/utils/archetypes/ordered_value_type.h
line 14 at r2 (raw file):
Previously, Marsella8 wrote…
the
libassert
asserts seem to be a lot nicer trace-wise compared to thecassert
ones, maybe worth it to move all assert logic tolibassert
? We could also use-DLIBASSERT_LOWERCASE
to aliasassert
tolibassert
s assert. (Obviously for a future PR).
Yup, that's the plan! We may even replace mk_runtime_error
with libassert, as libassert allows us to override the error handling behavior so we could still do exception throwing but more easily and better error messages
Description of changes:
lib
(specificallycompiler
andutils
as a starting point)proj
for runningbenchmarks
(runproj benchmark
)libassert
as a dependency (not really related to benchmarking, but just bundled in with this PR)Related Issues:
Linked Issues:
Issues closed by this PR:
This change is