On 23/06/2023 19:28, Jan Hubicka wrote:
>>>
>>> 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?
Both can be used independently, and you might actually want to - for branch
coverage (was every branch taken at least once?) the profile-arc instrumentation
was sufficient, but redundant if you also want condition coverage. However,
profile-arcs also give you branch *counts* which is useful for data-driven
decisions.
Now, we might want to have --coverage imply profile-conditions.
>>> 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<unsigned> 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<block_info *>::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<arc_info *>::const_iterator it = line->branches.begin ();
>>> + it != line->branches.end (); it++)
>>> + ix += output_branch_count (f, ix, (*it));
>>> + }
>>>
>>> - ix = 0;
>>> - for (vector<arc_info *>::const_iterator it = line->branches.begin ();
>>> - it != line->branches.end (); it++)
>>> - ix += output_branch_count (f, ix, (*it));
>>> + if (flag_conditions)
>>> + {
>>> + for (vector<block_info *>::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
I kept the profile-arcs/profille-coverage inline, but for the other feedback you
can consider it applied. I'll create some new patches to address them, then
squash them post-review. Thanks!
Jørgen
