From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id BD22F3858414 for ; Fri, 23 Jun 2023 10:28:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BD22F3858414 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id E253028AEDD; Fri, 23 Jun 2023 12:28:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1; t=1687516127; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=v9Eljf7es108R4i3W8ijpRWKbUB7JwgJU3P6NsAnN1A=; b=ooOaWWGdRdHBKMyeu6mf7BTLCrK4HMCP8i0xdkz6d/+cG3FDzk8OD73eslG1aWMf8S0iG6 71LhPOdMj22Qd68Uh8IXwH781KssgGQYtxdixUGOgWCxLZ95iFoJzXkueCUoZSQ8293j1J sX6xd+V2h3Z1bDm2lzGqqYfO/E0dJtI= Date: Fri, 23 Jun 2023 12:28:47 +0200 From: Jan Hubicka To: =?utf-8?Q?J=C3=B8rgen?= Kvalsvik Cc: gcc-patches@gcc.gnu.org Subject: Re: Ping [PATCH v4] Add condition coverage profiling Message-ID: References: <20230613005921.93315-1-jorgen.kvalsvik@woven.toyota> <515e91fc-7822-732c-9ea9-8c1305ba6562@woven.toyota> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <515e91fc-7822-732c-9ea9-8c1305ba6562@woven.toyota> X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > > > > gcc/ChangeLog: > > > > * builtins.cc (expand_builtin_fork_or_exec): Check > > profile_condition_flag. > > * collect2.cc (main): Add -fno-profile-conditions to OBSTACK. > > * common.opt: Add new options -fprofile-conditions and > > * doc/gcov.texi: Add --conditions documentation. > > * doc/invoke.texi: Add -fprofile-conditions documentation. > > * gcc.cc: Link gcov on -fprofile-conditions. > > * gcov-counter.def (GCOV_COUNTER_CONDS): New. > > * gcov-dump.cc (tag_conditions): New. > > * gcov-io.h (GCOV_TAG_CONDS): New. > > (GCOV_TAG_CONDS_LENGTH): Likewise. > > (GCOV_TAG_CONDS_NUM): Likewise. > > * gcov.cc (class condition_info): New. > > (condition_info::condition_info): New. > > (condition_info::popcount): New. > > (struct coverage_info): New. > > (add_condition_counts): New. > > (output_conditions): New. > > (print_usage): Add -g, --conditions. > > (process_args): Likewise. > > (output_intermediate_json_line): Output conditions. > > (read_graph_file): Read conditions counters. > > (read_count_file): Read conditions counters. > > (file_summary): Print conditions. > > (accumulate_line_info): Accumulate conditions. > > (output_line_details): Print conditions. > > * ipa-inline.cc (can_early_inline_edge_p): Check > > profile_condition_flag. > > * ipa-split.cc (pass_split_functions::gate): Likewise. > > * passes.cc (finish_optimization_passes): Likewise. > > * profile.cc (find_conditions): New declaration. > > (cov_length): Likewise. > > (cov_blocks): Likewise. > > (cov_masks): Likewise. > > (cov_free): Likewise. > > (instrument_decisions): New. > > (read_thunk_profile): Control output to file. > > (branch_prob): Call find_conditions, instrument_decisions. > > (init_branch_prob): Add total_num_conds. > > (end_branch_prob): Likewise. > > * tree-profile.cc (struct conds_ctx): New. > > (CONDITIONS_MAX_TERMS): New. > > (EDGE_CONDITION): New. > > (cmp_index_map): New. > > (index_of): New. > > (block_conditional_p): New. > > (edge_conditional_p): New. > > (single): New. > > (single_edge): New. > > (contract_edge): New. > > (contract_edge_up): New. > > (ancestors_of): New. > > (struct outcomes): New. > > (conditional_succs): New. > > (condition_index): New. > > (masking_vectors): New. > > (cond_reachable_from): New. > > (neighborhood): New. > > (isolate_expression): New. > > (emit_bitwise_op): New. > > (make_index_map_visit): New. > > (make_index_map): New. > > (collect_conditions): New. > > (yes): New. > > (struct condcov): New. > > (cov_length): New. > > (cov_blocks): New. > > (cov_masks): New. > > (cov_free): New. > > (find_conditions): New. > > (instrument_decisions): New. > > (tree_profiling): Check profile_condition_flag. > > (pass_ipa_tree_profile::gate): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > * lib/gcov.exp: Add condition coverage test function. > > * g++.dg/gcov/gcov-18.C: New test. > > * gcc.misc-tests/gcov-19.c: New test. > > * gcc.misc-tests/gcov-20.c: New test. > > * gcc.misc-tests/gcov-21.c: New test. > > --- > > > > v2: > > * Moved the docs to rst/sphinx > > * Output and message uses the 'conditions outcomes' vocabulary > > * Fixed errors reported by contrib/style-check. Note that a few > > warnings persist but are either in comments (ascii art) or because > > the surrounding code (typically lists) are formatted the same way > > v3: > > * Revert docs from rst/sphinx to texinfo > > v4: > > * Rebased on trunk, removed @gol from texi > > > > gcc/builtins.cc | 2 +- > > gcc/collect2.cc | 7 +- > > gcc/common.opt | 8 + > > gcc/doc/gcov.texi | 37 + > > gcc/doc/invoke.texi | 19 + > > gcc/gcc.cc | 4 +- > > gcc/gcov-counter.def | 3 + > > gcc/gcov-dump.cc | 24 + > > gcc/gcov-io.h | 3 + > > gcc/gcov.cc | 200 +++- > > gcc/ipa-inline.cc | 2 +- > > gcc/ipa-split.cc | 3 +- > > gcc/passes.cc | 3 +- > > gcc/profile.cc | 84 +- > > gcc/testsuite/g++.dg/gcov/gcov-18.C | 234 +++++ > > gcc/testsuite/gcc.misc-tests/gcov-19.c | 1250 ++++++++++++++++++++++++ > > gcc/testsuite/gcc.misc-tests/gcov-20.c | 22 + > > gcc/testsuite/gcc.misc-tests/gcov-21.c | 16 + > > gcc/testsuite/lib/gcov.exp | 191 +++- > > gcc/tree-profile.cc | 1048 +++++++++++++++++++- > > libgcc/libgcov-merge.c | 5 + > > 21 files changed, 3137 insertions(+), 28 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/gcov/gcov-18.C > > create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-19.c > > create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-20.c > > create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-21.c > > > > diff --git a/gcc/builtins.cc b/gcc/builtins.cc > > index 8400adaf5b4..c71124a5615 100644 > > --- a/gcc/builtins.cc > > +++ b/gcc/builtins.cc > > @@ -5902,7 +5902,7 @@ expand_builtin_fork_or_exec (tree fn, tree exp, rtx target, int ignore) > > tree call; > > > > /* If we are not profiling, just call the function. */ > > - if (!profile_arc_flag) > > + if (!profile_arc_flag && !profile_condition_flag) I think profile_condition_flag can simpy imply profile_arc_flag. This is an information you always measure in adition to the usual bb profile, right? This can be arranged in opts.c and would avoid need to test both flags throughout the compiler. Or can both profiles be used independently? > > return NULL_RTX; > > > > /* Otherwise call the wrapper. This should be equivalent for the rest of > > diff --git a/gcc/collect2.cc b/gcc/collect2.cc > > index 63b9a0c233a..12ff5d81424 100644 > > --- a/gcc/collect2.cc > > +++ b/gcc/collect2.cc > > @@ -1032,9 +1032,9 @@ main (int argc, char **argv) > > lto_mode = LTO_MODE_LTO; > > } > > > > - /* -fno-profile-arcs -fno-test-coverage -fno-branch-probabilities > > - -fno-exceptions -w -fno-whole-program */ > > - num_c_args += 6; > > + /* -fno-profile-arcs -fno-profile-conditions -fno-test-coverage > > + -fno-branch-probabilities -fno-exceptions -w -fno-whole-program */ > > + num_c_args += 7; > > > > c_argv = XCNEWVEC (char *, num_c_args); > > c_ptr = CONST_CAST2 (const char **, char **, c_argv); > > @@ -1230,6 +1230,7 @@ main (int argc, char **argv) > > } > > obstack_free (&temporary_obstack, temporary_firstobj); > > *c_ptr++ = "-fno-profile-arcs"; > > + *c_ptr++ = "-fno-profile-conditions"; If this was initialized from an array of strings, we can avoid need for num_c_args to be computed by hand.... > > *c_ptr++ = "-fno-test-coverage"; > > *c_ptr++ = "-fno-branch-probabilities"; > > *c_ptr++ = "-fno-exceptions"; > > diff --git a/gcc/common.opt b/gcc/common.opt > > index a28ca13385a..e32a5f7b0db 100644 > > --- a/gcc/common.opt > > +++ b/gcc/common.opt > > @@ -862,6 +862,10 @@ Wcoverage-invalid-line-number > > Common Var(warn_coverage_invalid_linenum) Init(1) Warning > > Warn in case a function ends earlier than it begins due to an invalid linenum macros. > > > > +Wcoverage-too-many-conditions > > +Common Var(warn_too_many_conditions) Init(1) Warning > > +Warn when a conditional has too many terms and coverage gives up. Maybe when user reads it in --help, he/she will not know what coverage you means. It is called condition coverage profiling later, so we can stick to this term here. > > + > > Wmissing-profile > > Common Var(warn_missing_profile) Init(1) Warning > > Warn in case profiles in -fprofile-use do not exist. > > @@ -2351,6 +2355,10 @@ fprofile-arcs > > Common Var(profile_arc_flag) > > Insert arc-based program profiling code. > > > > +fprofile-conditions > > +Common Var(profile_condition_flag) > > +Insert condition coverage profiling code. > > + > > fprofile-dir= > > Common Joined RejectNegative Var(profile_data_prefix) > > Set the top-level directory for storing the profile data. > > diff --git a/gcc/doc/gcov.texi b/gcc/doc/gcov.texi > > index 3019efc4674..10cfdcf24aa 100644 > > --- a/gcc/doc/gcov.texi > > +++ b/gcc/doc/gcov.texi > > @@ -124,6 +124,7 @@ gcov [@option{-v}|@option{--version}] [@option{-h}|@option{--help}] > > [@option{-a}|@option{--all-blocks}] > > [@option{-b}|@option{--branch-probabilities}] > > [@option{-c}|@option{--branch-counts}] > > + [@option{-g}|@option{--conditions}] > > [@option{-d}|@option{--display-progress}] > > [@option{-f}|@option{--function-summaries}] > > [@option{-j}|@option{--json-format}] > > @@ -169,6 +170,13 @@ be shown, unless the @option{-u} option is given. > > Write branch frequencies as the number of branches taken, rather than > > the percentage of branches taken. > > > > +@item -g > > +@itemx --conditions > > +Write condition coverage to the output file, and write condition summary info > > +to the standard output. This option allows you to see if the conditions in > > +your program at least once had an independent effect on the outcome of the > > +boolean expression (modified condition/decision coverage). Maybe you want to like -fprofile-conditions flag here, since that is needed at the instrumentaiton time. > > +@opindex Wno-coverage-too-many-conditions > > +@opindex Wcoverage-too-many-conditions > > +@item -Wno-coverage-too-many-conditions > > +Warn in case a condition have too many terms and GCC gives up coverage. > > +Coverage is given up when there are more terms in the conditional than there > > +are bits in a @code{gcov_type_unsigned}. This warning is enabled by default. Here you can explicitly mention -fprofile-conditions so user knows what kind of coverage you refer to. > > + > > @opindex Wno-coverage-invalid-line-number > > @opindex Wcoverage-invalid-line-number > > @item -Wno-coverage-invalid-line-number > > @@ -16430,6 +16438,13 @@ Note that if a command line directly links source files, the corresponding > > E.g. @code{gcc a.c b.c -o binary} would generate @file{binary-a.gcda} and > > @file{binary-b.gcda} files. > > > > +@item -fprofile-conditions > > +@opindex fprofile-conditions > > +Add code so that program conditions are instrumented. During execution the > > +program records what terms in a conditional contributes to a decision. The > > +data may be used to verify that all terms in a booleans are tested and have an > > +effect on the outcome of a condition. I would say that the result can be read with gcov --conditions > > @@ -392,6 +394,28 @@ tag_arcs (const char *filename ATTRIBUTE_UNUSED, > > } > > } > > > > +static void > > +tag_conditions (const char *filename ATTRIBUTE_UNUSED, > > + unsigned tag ATTRIBUTE_UNUSED, int length ATTRIBUTE_UNUSED, > > + unsigned depth) Coding style wants us to have a comment before functions summarizing what they do. Also with C++, one can ust skip "filename ATTRIBUTE_UNUSED" for unused args. > > diff --git a/gcc/gcov.cc b/gcc/gcov.cc > > index 2fad6aa7ede..ffedbb37483 100644 > > --- a/gcc/gcov.cc > > +++ b/gcc/gcov.cc > > @@ -81,6 +81,7 @@ using namespace std; > > class function_info; > > class block_info; > > class source_info; > > +class condition_info; > > > > /* Describes an arc between two basic blocks. */ > > > > @@ -134,6 +135,28 @@ public: > > vector lines; > > }; > > > > +class condition_info Also block comment here. > > +{ > > +public: > > + condition_info (); > > + > > + int popcount () const; > > + > > + gcov_type_unsigned truev; > > + gcov_type_unsigned falsev; > > + > > + unsigned n_terms; > > +}; > > + > > +condition_info::condition_info (): truev (0), falsev (0), n_terms (0) > > +{ > > +} > > + > > +int condition_info::popcount () const > > +{ > > + return __builtin_popcountll (truev) + __builtin_popcountll (falsev); If you build with --disable-bootstrap and use non-GCC host compiler, I don't think we have a promise of __builtin_popcountll being supported. THere is popcount_hwi in hwint.h > > @@ -2456,6 +2573,13 @@ add_branch_counts (coverage_info *coverage, const arc_info *arc) > > } > > } > > > > +static void > > +add_condition_counts (coverage_info *coverage, const block_info *block) Similarly here block comment will make coding conventions happy > > +{ > > + coverage->conditions += 2 * block->conditions.n_terms; > > + coverage->conditions_covered += block->conditions.popcount (); > > +} > > + > > /* Format COUNT, if flag_human_readable_numbers is set, return it human > > readable format. */ > > > > @@ -2559,6 +2683,18 @@ file_summary (const coverage_info *coverage) > > coverage->calls); > > else > > fnotice (stdout, "No calls\n"); > > + > > + } > > + > > + if (flag_conditions) > > + { > > + if (coverage->conditions) > > + fnotice (stdout, "Condition outcomes covered:%s of %d\n", > > + format_gcov (coverage->conditions_covered, > > + coverage->conditions, 2), > > + coverage->conditions); > > + else > > + fnotice (stdout, "No conditions\n"); > > } > > } > > > > @@ -2793,6 +2929,12 @@ static void accumulate_line_info (line_info *line, source_info *src, > > it != line->branches.end (); it++) > > add_branch_counts (&src->coverage, *it); > > > > + if (add_coverage) > > + for (vector::iterator it = line->blocks.begin (); > > + it != line->blocks.end (); it++) > > + add_condition_counts (&src->coverage, *it); > > + > > + > > if (!line->blocks.empty ()) > > { > > /* The user expects the line count to be the number of times > > @@ -2894,6 +3036,33 @@ accumulate_line_counts (source_info *src) > > } > > } > > > > +static void > > +output_conditions (FILE *gcov_file, const block_info *binfo) Again, I think we want a comment (even though it is easy to figure out what it does). > > +{ > > + const condition_info& info = binfo->conditions; > > + if (info.n_terms == 0) > > + return; > > + > > + const int expected = 2 * info.n_terms; > > + const int got = info.popcount (); > > + > > + fnotice (gcov_file, "condition outcomes covered %d/%d\n", got, expected); > > + if (expected == got) > > + return; > > + > > + for (unsigned i = 0; i < info.n_terms; i++) > > + { > > + gcov_type_unsigned index = 1; > > + index <<= i; > > + if ((index & info.truev & info.falsev)) > > + continue; > > + > > + const char *t = (index & info.truev) ? "" : "true"; > > + const char *f = (index & info.falsev) ? "" : " false"; > > + fnotice (gcov_file, "condition %2u not covered (%s%s)\n", i, t, f + !t[0]); > > + } > > +} > > + > > /* Output information about ARC number IX. Returns nonzero if > > anything is output. */ > > > > @@ -3104,16 +3273,29 @@ output_line_details (FILE *f, const line_info *line, unsigned line_num) > > if (flag_branches) > > for (arc = (*it)->succ; arc; arc = arc->succ_next) > > jx += output_branch_count (f, jx, arc); > > + > > + if (flag_conditions) > > + output_conditions (f, *it); > > } > > } > > - else if (flag_branches) > > + else > > { > > - int ix; > > + if (flag_branches) > > + { > > + int ix; > > + > > + ix = 0; > > + for (vector::const_iterator it = line->branches.begin (); > > + it != line->branches.end (); it++) > > + ix += output_branch_count (f, ix, (*it)); > > + } > > > > - ix = 0; > > - for (vector::const_iterator it = line->branches.begin (); > > - it != line->branches.end (); it++) > > - ix += output_branch_count (f, ix, (*it)); > > + if (flag_conditions) > > + { > > + for (vector::const_iterator it = line->blocks.begin (); > > + it != line->blocks.end (); it++) > > + output_conditions (f, *it); > > + } The gcov tool bits seems good to me except the nits above. I will continue reading the instrumentation part and write incrementally. Honza