> gcc/ChangeLog:
>
> * builtins.cc (expand_builtin_fork_or_exec): Check
> condition_coverage_flag.
> * collect2.cc (main): Add -fno-condition-coverage to OBSTACK.
> * common.opt: Add new options -fcondition-coverage and
> -Wcoverage-too-many-conditions.
> * doc/gcov.texi: Add --conditions documentation.
> * doc/invoke.texi: Add -fcondition-coverage documentation.
> * function.cc (free_after_compilation): Clear conditions.
> (allocate_struct_function): Allocate conditions.
> (basic_condition_uid): New.
> * function.h (struct function): Add conditions.
> (basic_condition_uid): New declaration.
> * gcc.cc: Link gcov on -fcondition-coverage.
> * gcov-counter.def (GCOV_COUNTER_CONDS): New.
> * gcov-dump.cc (tag_conditions): New.
> * gcov-io.h (GCOV_TAG_CONDS): New.
> (GCOV_TAG_CONDS_LENGTH): New.
> (GCOV_TAG_CONDS_NUM): New.
> * 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 condition counters.
> (read_count_file): Likewise.
> (file_summary): Print conditions.
> (accumulate_line_info): Accumulate conditions.
> (output_line_details): Print conditions.
> * gimplify.cc (next_cond_uid): New.
> (reset_cond_uid): New.
> (shortcut_cond_r): Set condition discriminator.
> (tag_shortcut_cond): New.
> (shortcut_cond_expr): Set condition discriminator.
> (gimplify_cond_expr): Likewise.
> (gimplify_function_tree): Call reset_cond_uid.
> * ipa-inline.cc (can_early_inline_edge_p): Check
> condition_coverage_flag.
> * ipa-split.cc (pass_split_functions::gate): Likewise.
> * passes.cc (finish_optimization_passes): Likewise.
> * profile.cc (struct condcov): New declaration.
> (cov_length): Likewise.
> (cov_blocks): Likewise.
> (cov_masks): Likewise.
> (cov_maps): 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-core.h (struct tree_exp): Add condition_uid.
> * tree-profile.cc (struct conds_ctx): New.
> (CONDITIONS_MAX_TERMS): New.
> (EDGE_CONDITION): New.
> (topological_cmp): New.
> (index_of): New.
> (single_p): New.
> (single_edge): New.
> (contract_edge_up): New.
> (struct outcomes): New.
> (conditional_succs): New.
> (condition_index): New.
> (masking_vectors): New.
> (emit_assign): New.
> (emit_bitwise_op): New.
> (make_top_index_visit): New.
> (make_top_index): New.
> (paths_between): New.
> (struct condcov): New.
> (cov_length): New.
> (cov_blocks): New.
> (cov_masks): New.
> (cov_maps): New.
> (cov_free): New.
> (gimple_cond_uid): New.
> (find_conditions): New.
> (struct counters): New.
> (find_counters): New.
> (resolve_counter): New.
> (resolve_counters): New.
> (instrument_decisions): New.
> (tree_profiling): Check condition_coverage_flag.
> (pass_ipa_tree_profile::gate): Likewise.
> * tree.h (SET_EXPR_UID): New.
> (EXPR_COND_UID): New.
>
> libgcc/ChangeLog:
>
> * libgcov-merge.c (__gcov_merge_ior): New.
>
> 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.
> * gcc.misc-tests/gcov-22.c: New test.
> * gcc.misc-tests/gcov-23.c: New test.
Sorry for taking so long on this - I needed some time to actually try
the patch since generally we will need more changes in frontend to
preserve conditionals intanct till gimple.
> This revision brings quite a few changes, some of which warrant a more
> careful review.
>
> 1. Basic conditions are tied to the Boolean expression during
> gimplification, not through CFG analysis. The CFG analysis seemed to
> work well up until constructs like a && fn (b && c) && d where
> fn(...) seems indistinguishible from then-blocks. This wipes out much
> of the implementation in tree-profile.cc.
> 2. I changed the flag from -fprofile-conditions to -fcondition-coverage.
> -fprofile-conditions was chosen because of its symmetry with
> -fprofile-arcs, condition-coverage does feel more appropriate.
This seems good. Profile-arcs is rarely used by itself - most of time it
is implied by -fprofile-generate and -ftest-coverage and since
condition coverage is more associated to the second, I guess
-fcondition-coverage is better name.
Since -fcondition-coverage now affects gimplification (which makes me
less worried about its ability to map things back to conditionals), it
should be marked as incompatible with -fprofile-generate and
-fprofile-use. It would only work with -fcondtion-coverage was used
both with -fprofile-generate and -fprofile-use and I do not see much use
of this. On the other hand I can imagine users wanting both coverage and
-fprofile-generate run at once, so we should output sorry message that
this is not implemented
> --- a/gcc/function.h
> +++ b/gcc/function.h
> @@ -287,6 +287,10 @@ struct GTY(()) function {
> /* Vector of function local variables, functions, types and constants. */
> vec<tree, va_gc> *local_decls;
>
> + /* Map from conditions to a (function) unique identifier, so that two basic
> + conditions that belong to the same expression have the same id. */
> + hash_map<gcond *, unsigned> *conditions;
> +
> /* For md files. */
>
> /* tm.h can use this to store whatever it likes. */
> @@ -737,4 +741,6 @@ extern void used_types_insert (tree);
>
> extern bool currently_expanding_function_start;
>
> +extern unsigned basic_condition_uid (const struct function *, basic_block);
> +
> #endif /* GCC_FUNCTION_H */
> diff --git a/gcc/function.cc b/gcc/function.cc
> index 89841787ff8..5319b182a6a 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -85,6 +85,7 @@ along with GCC; see the file COPYING3. If not see
> #include "value-range.h"
> #include "gimple-range.h"
> #include "insn-attr.h"
> +#include "gimple-iterator.h"
>
> /* So we can assign to cfun in this file. */
> #undef cfun
> @@ -213,6 +214,7 @@ free_after_compilation (struct function *f)
>
> memset (crtl, 0, sizeof (struct rtl_data));
> f->eh = NULL;
> + f->conditions = NULL;
> f->machine = NULL;
> f->cfg = NULL;
> f->curr_properties &= ~PROP_cfg;
> @@ -2385,7 +2387,7 @@ split_complex_args (vec<tree> *args)
> the hidden struct return argument, and (abi willing) complex args.
> Return the new parameter list. */
>
> -static vec<tree>
> +static vec<tree>
> assign_parms_augmented_arg_list (struct assign_parm_data_all *all)
> {
> tree fndecl = current_function_decl;
> @@ -4794,6 +4796,7 @@ allocate_struct_function (tree fndecl, bool abstract_p)
> cfun = ggc_cleared_alloc<function> ();
>
> init_eh_for_function ();
> + cfun->conditions = hash_map<gcond*, unsigned>::create_ggc ();
>
> if (init_machine_status)
> cfun->machine = (*init_machine_status) ();
> @@ -6977,6 +6980,22 @@ add_local_decl (struct function *fun, tree d)
> vec_safe_push (fun->local_decls, d);
> }
>
> +/* Get the basic condition identifier for the basic block in the
> + function fn, which is set in gimplification of conditional expressions.
> If
> + a basic block does not end with a conditional jump, or the jump was
> created
> + implicitly (e.g. from a C++ destructor) this function returns 0. */
> +unsigned
> +basic_condition_uid (const struct function *fn, basic_block bb)
> +{
> + gimple *stmt = gsi_stmt (gsi_last_nondebug_bb (bb));
> + if (!stmt)
> + return 0;
> + if (!is_a<gcond *> (stmt))
> + return 0;
> + unsigned *v = fn->conditions->get (as_a<gcond *> (stmt));
> + return v ? *v : 0;
> +}
> +
If condition statement is removed before profiling is done, you will end
up with a stale entry in the hash table.
We already keep EH regions and profile histogram in on-side hashtables,
so I think you need to follow gimple_.*histograms calls in
gimple-iterator.cc and be sure that the hashtable is updated.
Also if function is inlined (which may be forced at -O0 using
always_inline) the conditional annotations will be lost.
I think you should simply make condition coverage to happen before
pass_early_inline is run. But that may be done incrementally.
> namespace {
>
> const pass_data pass_data_match_asm_constraints =
> diff --git a/gcc/function.h b/gcc/function.h
> index 833c35e3da6..a3ce5a70aa8 100644
> --- a/gcc/function.h
> +++ b/gcc/function.h
> @@ -287,6 +287,10 @@ struct GTY(()) function {
> /* Vector of function local variables, functions, types and constants. */
> vec<tree, va_gc> *local_decls;
>
> + /* Map from conditions to a (function) unique identifier, so that two basic
> + conditions that belong to the same expression have the same id. */
> + hash_map<gcond *, unsigned> *conditions;
> +
> /* For md files. */
>
> /* tm.h can use this to store whatever it likes. */
> @@ -737,4 +741,6 @@ extern void used_types_insert (tree);
>
> extern bool currently_expanding_function_start;
>
> +extern unsigned basic_condition_uid (const struct function *, basic_block);
> +
> #endif /* GCC_FUNCTION_H */
> +/* Describes a single conditional expression and the (recorded) conditions
> + shown to independently affect the outcome. */
> +class condition_info
> +{
> +public:
> + condition_info ();
> +
> + int popcount () const;
> +
> + /* Bitsets storing the independently significant outcomes for true and
> false,
> + * respectively. */
^ extra * ..
I skipped the gcov.cc changes - I suppose they did not changed
significantly since last version of patch, right?
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index 342e43a7f25..6b5f604eecf 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -71,6 +71,14 @@ along with GCC; see the file COPYING3. If not see
> #include "context.h"
> #include "tree-nested.h"
>
> +/* Initial value for the condition identifier, which it can be reset to
> + on when entering functions. 0 is already the default/untouched value, so
> + start at non-zero. A valid and set id should always be > 0. */
Please add mention of condition coverage to the comment, so it is clear
what kind of identifier we look for here.
> +static const unsigned initial_cond_uid = 1;
> +static unsigned nextuid = 0;
> +unsigned next_cond_uid () { return nextuid++; }
> +void reset_cond_uid () { nextuid = initial_cond_uid; }
and reformat this to gnu style (with extra lines for { return and }.
> +
> /* Hash set of poisoned variables in a bind expr. */
> static hash_set<tree> *asan_poisoned_variables = NULL;
>
> @@ -4137,7 +4145,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p,
> bool want_value)
>
> static tree
> shortcut_cond_r (tree pred, tree *true_label_p, tree *false_label_p,
> - location_t locus)
> + location_t locus, unsigned condition_uid)
You should document the new parameter (and make clear it is associated
to condition coverage. */
> +
> +/* Given a multi-term condition (ANDIF, ORIF), walk the predicate and tag
> every
> + term with uid. When two basic conditions share the uid discriminator when
> + they belong to the same predicate, which used by the condition coverage.
> + Doing this as an explicit step makes for a simpler implementation than
> + weaving it into the splitting code as the splitting code eventually
> reaches
> + back up to gimplfiy_expr which makes bookkeeping complicated. */
> +static void
> +tag_shortcut_cond (tree pred, unsigned condition_uid)
> +{
> + if (TREE_CODE (pred) == TRUTH_ANDIF_EXPR
> + || TREE_CODE (pred) == TRUTH_ORIF_EXPR)
> + {
> + tree fst = TREE_OPERAND (pred, 0);
> + tree lst = TREE_OPERAND (pred, 1);
> +
> + if (TREE_CODE (fst) == TRUTH_ANDIF_EXPR
> + || TREE_CODE (fst) == TRUTH_ORIF_EXPR)
> + tag_shortcut_cond (fst, condition_uid);
> + else if (TREE_CODE (fst) == COND_EXPR)
> + SET_EXPR_UID (fst, condition_uid);
> +
> + if (TREE_CODE (lst) == TRUTH_ANDIF_EXPR
> + || TREE_CODE (lst) == TRUTH_ORIF_EXPR)
> + tag_shortcut_cond (lst, condition_uid);
> + else if (TREE_CODE (lst) == COND_EXPR)
> + SET_EXPR_UID (lst, condition_uid);
So you store the uids into tree expression to later copy it to gimple.
It is not a good idea to add condition_uid into core tree type:
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index 65e51b939a2..44b9fa10431 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1582,6 +1582,9 @@ enum omp_clause_linear_kind
> struct GTY(()) tree_exp {
> struct tree_typed typed;
> location_t locus;
> + /* Discriminator for basic conditions in a Boolean expressions. Trees that
> + are operands of the same Boolean expression should have the same uid.
> */
> + unsigned condition_uid;
> tree GTY ((length ("TREE_OPERAND_LENGTH ((tree)&%h)"))) operands[1];
> };
Instead of that it can live in on-side hashtable, since most of time
(when condition coverage is off) it will not be necessary.
You can see how this is done with decl_debug_expr_lookup and
decl_debug_expr_insert that is kind of similar situation.
I would like to see the patch with updated and Richard to have chance to
comment on gimplify.cc/function.cc and tree.h changes, but otherwise I
think the patch is ready to go.
Honza
