On 29/12/2023 22:14, Jan Hubicka wrote:
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
I don't quite understand this - gimplification is affected only in the sense that slightly more information is recorded, why would it be incompatible with -fprofile-generate?
In the v8 revision I switched strategy to not using an auxiliary hash table, but rather temporarily in the gimple.uid field until the basic block is created and it can be stored there. What do you think about that approach?--- 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.
https://patchwork.sourceware.org/project/gcc/patch/20231212114125.1998866-...@lambda.is/I assume the stale entries is why I got ICEs from what looked like double frees in gc runs.
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?
Correct.
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 }.
Done.
+ /* 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 treeshortcut_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.
As you say, I ran into all sorts of trouble and different layout expectations with this. In v8 it is stored directly in the basic block, which feels like a much nicer design with less bookkeeping overall.
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
