Re: Ping [PATCH v4] Add condition coverage profiling

["Jørgen Kvalsvik" <jorgen.kvalsvik@woven.toyota>]

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