This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: FDO usability patch -- cfg and lineno checksum
I can not review tree.c changes. I would probably suggest making crc_byte inline.
> +#if IN_LIBGCOV
> +
> +/* These functions are guarded by #if to avoid compile time warning. */
> +
> +/* Return the number of words STRING would need including the length
> + field in the output stream itself. This should be identical to
> + "alloc" calculation in gcov_write_string(). */
Hmm, probably better to make gcov_write_string to use gcov_string_length then.
> @@ -238,13 +265,15 @@ gcov_write_words (unsigned words)
>
> gcc_assert (gcov_var.mode < 0);
> #if IN_LIBGCOV
> - if (gcov_var.offset >= GCOV_BLOCK_SIZE)
> + if (gcov_var.offset + words >= GCOV_BLOCK_SIZE)
> {
> - gcov_write_block (GCOV_BLOCK_SIZE);
> + gcov_write_block (MIN (gcov_var.offset, GCOV_BLOCK_SIZE));
> if (gcov_var.offset)
> {
> - gcc_assert (gcov_var.offset == 1);
> - memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
> + gcc_assert (gcov_var.offset < GCOV_BLOCK_SIZE);
> + memcpy (gcov_var.buffer,
> + gcov_var.buffer + GCOV_BLOCK_SIZE,
> + gcov_var.offset << 2);
I don't really follow the logic here. buffer is allocated to be size of
block+4 and it is expected that gcov_write_words is not executed on size
greater than 4. Since gcov_write_string now seems to be expected to handle
strings of bigger size, I think you acually need to make write_string to write
in chunks when you reach block boundary?
As soon as you decide to not write out in the GCOV_BLOCK_SIZE chunks, the
MIN computation above seems unnecesary (and bogus, since we won't let
gcov_var.offset to grow past GCOV_BLOCK_SIZE.
What gets into libgcov is very problematic busyness for embedded targets, where you really want
libgcov to be small. Why do you need to store strings now?
> Index: gcov-io.h
> ===================================================================
> --- gcov-io.h (revision 172693)
> +++ gcov-io.h (working copy)
> @@ -101,9 +101,10 @@ see the files COPYING3 and COPYING.RUNTI
>
> The basic block graph file contains the following records
> note: unit function-graph*
> - unit: header int32:checksum string:source
> + unit: header int32:checksum int32: string:source
> function-graph: announce_function basic_blocks {arcs | lines}*
> - announce_function: header int32:ident int32:checksum
> + announce_function: header int32:ident
> + int32:lineno_checksum int32:cfg_checksum
> string:name string:source int32:lineno
> basic_block: header int32:flags*
> arcs: header int32:block_no arc*
> @@ -132,7 +133,9 @@ see the files COPYING3 and COPYING.RUNTI
> data: {unit function-data* summary:object summary:program*}*
> unit: header int32:checksum
> function-data: announce_function arc_counts
> - announce_function: header int32:ident int32:checksum
> + announce_function: header int32:ident
> + int32:lineno_checksum int32:cfg_checksum
> + string:name
> arc_counts: header int64:count*
> summary: int32:checksum {count-summary}GCOV_COUNTERS
> count-summary: int32:num int32:runs int64:sum
We also need to bump gcov version, right?
> @@ -411,11 +417,20 @@ struct gcov_summary
> /* Information about a single function. This uses the trailing array
> idiom. The number of counters is determined from the counter_mask
> in gcov_info. We hold an array of function info, so have to
> - explicitly calculate the correct array stride. */
> + explicitly calculate the correct array stride.
> +
> + "ident" is no longer used in hash computation. Additionally,
> + "name" is used in hash computation. This makes the profile data
> + not compatible across function name changes.
> + Also, the function pointer is stored for later use for indirect
> + function call name resolution.
Hmm, your patch is not adding indirect function call name resolution,
so independent change?
> Index: profile.c
> ===================================================================
> --- profile.c (revision 172693)
> +++ profile.c (working copy)
> @@ -102,13 +102,6 @@ static int total_num_branches;
>
> /* Forward declarations. */
> static void find_spanning_tree (struct edge_list *);
> -static unsigned instrument_edges (struct edge_list *);
> -static void instrument_values (histogram_values);
> -static void compute_branch_probabilities (void);
> -static void compute_value_histograms (histogram_values);
> -static gcov_type * get_exec_counts (void);
> -static basic_block find_group (basic_block);
> -static void union_groups (basic_block, basic_block);
This is also independent and OK for mainline.
>
> /* Add edge instrumentation code to the entire insn chain.
>
> @@ -233,10 +226,12 @@ instrument_values (histogram_values valu
> }
>
>
> -/* Computes hybrid profile for all matching entries in da_file. */
> +/* Computes hybrid profile for all matching entries in da_file.
> +
> + CFG_CHECKSUM is the precomputed checksum for the CFG. */
document LINENO_CHECKSUM, too.
> struct function_list *next; /* next function */
> unsigned ident; /* function ident */
> - unsigned checksum; /* function checksum */
> + unsigned lineno_checksum; /* function lineno checksum */
> + unsigned cfg_checksum; /* function cfg checksum */
> + const char *name; /* function name */
> + tree decl; /* function decl */
> unsigned n_ctrs[GCOV_COUNTERS];/* number of counters. */
> };
>
> @@ -67,9 +71,11 @@ typedef struct counts_entry
> /* We hash by */
> unsigned ident;
> unsigned ctr;
> + char *name;
also const, right?
> @@ -139,12 +144,21 @@ get_gcov_unsigned_t (void)
> return lang_hooks.types.type_for_size (32, true);
> }
>
> +/* Return the type node for const char *. */
> +
> +static tree
> +get_const_string_type (void)
This is not really coverage specific and already mudflap is doing the same. So probably it better
should go into tree.c.
> - if (entry->checksum != checksum)
> - inform (input_location, "checksum is %x instead of %x",
> - entry->checksum, checksum);
> + if (entry->cfg_checksum != cfg_checksum)
> + inform (input_location, "cfg_checksum is %x instead of %x",
> + entry->cfg_checksum, cfg_checksum);
Well, the message is already cryptic, perhaps just expanding cfg to be control flow graph
so users get some clue.
> @@ -641,13 +744,24 @@ build_fn_info_type (unsigned int counter
> /* ident */
> fields = build_decl (BUILTINS_LOCATION,
> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> -
> - /* checksum */
> + /* lineno_checksum */
> field = build_decl (BUILTINS_LOCATION,
> FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> DECL_CHAIN (field) = fields;
> fields = field;
>
> + /* cfg checksum */
> + field = build_decl (BUILTINS_LOCATION,
> + FIELD_DECL, NULL_TREE, get_gcov_unsigned_t ());
> + DECL_CHAIN (field) = fields;
> + fields = field;
Why do we need function names to end up there at runtime?
Honza