> + /* For a given index, returns the symbol name. */
> + const char *get_symbol_name (int index) const;
> +
> + /* For a given index, returns the filename. */
> + const char *get_filename (int index) const;
> +
> + /* For a given symbol name index, returns the filename index. */
> + int get_filename_by_symbol (int index) const;
> +
> + /* For a given function name, returns the filename index. */
> + int get_filename_by_symbol (const char *name) const;
When we need this? This seems kind of dangerous given that map is not
1-to-1
> public:
> + class descriptor
Please add comment what descriptor does
(it is kind of obvious but one wonders why this is a subclass)
> + {
> + /* The string_table index for the file name. */
> + unsigned file_name_;
> + /* The string_table index for the function name. */
> + unsigned symbol_name_;
> +
> + public:
> + unsigned file_name () const { return file_name_; }
> + unsigned symbol_name () const { return symbol_name_; }
> +
> + descriptor (unsigned file_name, unsigned symbol_name)
> + : file_name_ (file_name), symbol_name_ (symbol_name)
> + {}
> +
> + descriptor (int file_name, int symbol_name)
> + : file_name_ (file_name), symbol_name_ (symbol_name)
> + {}
> +
> + unsigned set_symbol_name (unsigned new_name)
> + {
> + return symbol_name_ = new_name;
> + }
Probably this can be void?
Similarly for other setters.
> -string_table::get_name (int index) const
> +string_table::get_symbol_name (int index) const
> +{
> + gcc_assert (index > 0 && index < (int) symbol_names_.length ());
Use gcc_checking_assert for those cheap checks. Autofdo stream in can
get slow.
We probably also want to watch situation where wrong indices may come
from file itslf and output readable errors about corrupted profile data
rather than ICEs/segfaults.
> + return symbol_names_[index];
> +}
> +
> +/* For a given index, returns the string. */
> +
> +const char *
> +string_table::get_filename (int index) const
> +{
> + /* There may not be any file name for some functions, ignore them. */
> + if (index == string_table::unknown_filename)
> + return "<unknown>";
> + gcc_assert (index >= 0 && index < (int) filenames_.length ());
> + return filenames_[index];
> +}
> +
> +/* For a given symbol name index, returns the filename index. */
> +
> +int
> +string_table::get_filename_by_symbol (int index) const
> {
> - gcc_assert (index > 0 && index < (int)vector_.length ());
> - return vector_[index];
> + return get_filename_by_symbol (get_symbol_name (index));
> }
>
> -/* Add new name SRRING and return its index. */
> +/* For a given function name, returns the filename index. */
>
> int
> -string_table::add_name (char *string)
> +string_table::get_filename_by_symbol (const char *name) const
> {
> - vector_.safe_push (string);
> - map_[vector_.last ()] = vector_.length () - 1;
> - return vector_.length () - 1;
> + auto it = symbol_to_filename_map_.find (name);
> + if (it != symbol_to_filename_map_.end () && it->second < filenames_.length
> ())
> + return it->second;
> + return string_table::unknown_filename;
> +}
> +
> +/* For a given filename, returns the index. */
> +
> +int
> +string_table::get_filename_index (const char *name) const
> +{
> + auto iter = filename_map_.find (name);
> + return iter == filename_map_.end () ? string_table::unknown_filename
> + : iter->second;
> +}
> +
> +/* Add new symbol name STRING (with an associated file name FILENAME_IDX) and
> + return its index. */
> +
> +int
> +string_table::add_symbol_name (const char *string, int filename_idx)
> +{
> + gcc_assert (
> + filename_idx == string_table::unknown_filename
> + || (filename_idx >= 0 && filename_idx < (int) filenames_.length ()));
> + symbol_names_.safe_push (string);
> + symbol_name_map_[symbol_names_.last ()] = symbol_names_.length () - 1;
> + symbol_to_filename_map_[symbol_names_.last ()] = filename_idx;
> + return symbol_names_.length () - 1;
> +}
> +
> +/* Add new filename and return its index (returning the same if it already
> + exists). */
> +
> +int
> +string_table::add_filename (const char *name)
> +{
> + auto it = filename_map_.find (name);
> + if (it != filename_map_.end ())
> + return it->second;
> + filenames_.safe_push (xstrdup (name));
> + return filenames_.length () - 1;
> }
>
> /* Read the string table. Return TRUE if reading is successful. */
> @@ -897,12 +1044,26 @@ string_table::read ()
> /* Skip the length of the section. */
> gcov_read_unsigned ();
> /* Read in the file name table. */
> + unsigned file_num = gcov_read_unsigned ();
> + filenames_.reserve (file_num);
> + for (unsigned i = 0; i < file_num; i++)
> + {
> + const char *filename = gcov_read_string ();
> + filenames_.quick_push (xstrdup (get_normalized_path (filename, true)));
> + filename_map_[filenames_.last ()] = i;
> + free (const_cast<char *> (filename));
> + if (gcov_is_error ())
> + return false;
> + }
> + /* Read in the function name -> file name table. */
> unsigned string_num = gcov_read_unsigned ();
> - vector_.reserve (string_num);
> + symbol_names_.reserve (string_num);
> for (unsigned i = 0; i < string_num; i++)
> {
> - vector_.quick_push (xstrdup (gcov_read_string ()));
> - map_[vector_.last ()] = i;
> + symbol_names_.quick_push (const_cast<char *> (gcov_read_string ()));
> + symbol_name_map_[symbol_names_.last ()] = i;
> + unsigned filename_idx = gcov_read_unsigned ();
> + symbol_to_filename_map_[symbol_names_.last ()] = filename_idx;
> if (gcov_is_error ())
> return false;
> }
> @@ -914,7 +1075,7 @@ string_table::read ()
> cgraph_node *
> string_table::get_cgraph_node (int name_index)
> {
> - const char *sname = get_name (name_index);
> + const char *sname = get_symbol_name (name_index);
>
> symtab_node *n = cgraph_node::get_for_asmname (get_identifier (sname));
> for (;n; n = n->next_sharing_asm_name)
> @@ -929,7 +1090,7 @@ string_table::get_cgraph_node (int name_index)
> cgraph_node *
> function_instance::get_cgraph_node ()
> {
> - return afdo_string_table->get_cgraph_node (name ());
> + return afdo_string_table->get_cgraph_node (symbol_name ());
> }
>
> /* Member functions for function_instance. */
> @@ -974,9 +1135,9 @@ function_instance::get_function_instance_by_decl
> (unsigned lineno,
> dump_user_location_t::from_location_t (location),
> "auto-profile has mismatched function name %s"
> " insteed of %s at loc %i:%i",
> - afdo_string_table->get_name (iter.first.second),
> - raw_symbol_name (decl),
> - lineno >> 16,
> + afdo_string_table->get_symbol_name (
> + iter.first.second),
> + raw_symbol_name (decl), lineno >> 16,
> lineno & 65535);
> }
>
> @@ -991,7 +1152,12 @@ function_instance::merge (function_instance *other,
> vec <function_instance *> &new_functions)
> {
> /* Do not merge to itself and only merge functions of same name. */
> - gcc_checking_assert (other != this && other->name () == name ());
> + gcc_checking_assert (other != this
> + && other->symbol_name () == symbol_name ());
> +
> + if (file_name () != other->file_name ())
> + return;
> +
> total_count_ += other->total_count_;
> if (other->total_count () && total_count () && other->head_count () == -1)
> head_count_ = -1;
> @@ -1111,7 +1277,8 @@ function_instance::offline (function_instance *fn,
> gcc_checking_assert (s->total_count_ >= 0);
> }
> function_instance *to
> - = afdo_source_profile->get_function_instance_by_name_index (fn->name ());
> + = afdo_source_profile->get_function_instance_by_descriptor (
> + fn->get_descriptor ());
> fn->set_inlined_to (NULL);
> /* If there is offline function of same name, we need to merge profile.
> Delay this by adding function to a worklist so we do not run into
> @@ -1186,7 +1353,8 @@ match_with_target (cgraph_node *n,
> {
> cgraph_node *callee = orig_callee->ultimate_alias_target ();
> const char *symbol_name = raw_symbol_name (callee->decl);
> - const char *name = afdo_string_table->get_name (inlined_fn->name ());
> + const char *name
> + = afdo_string_table->get_symbol_name (inlined_fn->symbol_name ());
> if (strcmp (name, symbol_name))
> {
> int i;
> @@ -1200,18 +1368,21 @@ match_with_target (cgraph_node *n,
> }
> /* Accept dwarf names and stripped suffixes. */
> if (!strcmp (lang_hooks.dwarf_name (callee->decl, 0),
> - afdo_string_table->get_name (inlined_fn->name ()))
> - || (!name[i] && symbol_name[i] == '.')
> - || in_suffix)
> + afdo_string_table->get_symbol_name (
> + inlined_fn->symbol_name ()))
> + || (!name[i] && symbol_name[i] == '.') || in_suffix)
> {
> int index = afdo_string_table->get_index (symbol_name);
> if (index == -1)
> - index = afdo_string_table->add_name (xstrdup (symbol_name));
> + index = afdo_string_table->add_symbol_name (
> + xstrdup (symbol_name),
> + afdo_string_table->add_filename (
> + get_normalized_path (DECL_SOURCE_FILE (callee->decl))));
> if (dump_file)
> fprintf (dump_file,
> " Renaming inlined call target %s to %s\n",
> name, symbol_name);
> - inlined_fn->set_name (index);
> + inlined_fn->set_symbol_name (index);
> return 2;
> }
> /* Only warn about declarations. It is possible that the function
> @@ -1493,10 +1664,9 @@ function_instance::match (cgraph_node *node,
> "auto-profile of %q+F seem to contain"
> " lost discriminator %i for"
> " call of %s at relative location %i",
> - node->decl,
> - loc & 65535,
> - afdo_string_table->get_name
> - (inlined_fn_nodisc->name ()),
> + node->decl, loc & 65535,
> + afdo_string_table->get_symbol_name (
> + inlined_fn_nodisc->symbol_name ()),
> loc >> 16))
> inform (gimple_location (stmt),
> "corresponding call");
> @@ -1512,19 +1682,18 @@ function_instance::match (cgraph_node *node,
> {
> if (inlined_fn)
> {
> - int old_name = inlined_fn->name ();
> + int old_name = inlined_fn->symbol_name ();
> int r = match_with_target (node, stmt, inlined_fn,
> callee_node);
> if (r == 2)
> {
> auto iter = callsites.find ({loc, old_name});
> - gcc_checking_assert (old_name
> - != inlined_fn->name ()
> - && iter != callsites.end ()
> - && iter->second
> - == inlined_fn);
> + gcc_checking_assert (
> + old_name != inlined_fn->symbol_name ()
> + && iter != callsites.end ()
> + && iter->second == inlined_fn);
> callsite key2 = {stack[0].afdo_loc,
> - inlined_fn->name ()};
> + inlined_fn->symbol_name ()};
> callsites.erase (iter);
> callsites[key2] = inlined_fn;
> }
> @@ -1575,23 +1744,24 @@ function_instance::match (cgraph_node *node,
> (gimple_location (stmt));
> /* Do renaming if needed so we can look up
> cgraph node and recurse into inlined function. */
> - int *newn = to_symbol_name.get (inlined_fn->name ());
> - gcc_checking_assert
> - (!newn || *newn != inlined_fn->name ());
> + int *newn
> + = to_symbol_name.get (inlined_fn->symbol_name ());
> + gcc_checking_assert (
> + !newn || *newn != inlined_fn->symbol_name ());
> if (newn || lost_discriminator)
> {
> - auto iter = callsites.find
> - ({loc, inlined_fn->name ()});
> + auto iter = callsites.find (
> + {loc, inlined_fn->symbol_name ()});
> gcc_checking_assert (iter != callsites.end ()
> && iter->second
> == inlined_fn);
> - callsite key2 = {stack[0].afdo_loc,
> - newn ? *newn
> - : inlined_fn->name ()};
> + callsite key2
> + = {stack[0].afdo_loc,
> + newn ? *newn : inlined_fn->symbol_name ()};
> callsites.erase (iter);
> callsites[key2] = inlined_fn;
> - inlined_fn->set_name (newn ? *newn
> - : inlined_fn->name ());
> + inlined_fn->set_symbol_name (
> + newn ? *newn : inlined_fn->symbol_name ());
> }
> functions.add (inlined_fn);
> }
> +/* Find the matching function instance which has DESCRIPTOR as its
> + descriptor. If not found, also try checking if an instance exists with
> the
> + same name which has no associated filename. */
I believe the unknown filename problems should go away if we fix the
dwarf abbrevs with autofdo. Have patch on that and will try to get it
into official tree.
Pathc is OK with the changes above.
thanks,
Honza
> +
> +autofdo_source_profile::name_function_instance_map::const_iterator
> +autofdo_source_profile::find_iter_for_function_instance (
> + function_instance::descriptor descriptor) const
> +{
> + auto it = map_.find (descriptor);
> +
> + /* Try searching for the symbol not having a filename if it isn't found.
> */
> + if (it == map_.end ())
> + it = map_.find (
> + function_instance::descriptor (string_table::unknown_filename,
> + (int) descriptor.symbol_name ()));
> + return it;
> +}
> +
> +/* Similar to the above, but return a pointer to the instance instead of an
> + iterator. */
> +
> +function_instance *
> +autofdo_source_profile::find_function_instance (
> + function_instance::descriptor descriptor) const
> +{
> + auto it = find_iter_for_function_instance (descriptor);
> + return it == map_.end () ? NULL : it->second;
> +}
> +
> +/* Remove a function instance from the map. Returns true if the entry was
> + actually deleted. */
> +
> +bool
> +autofdo_source_profile::remove_function_instance (function_instance *inst)
> +{
> + auto iter = find_iter_for_function_instance (inst->get_descriptor ());
> + if (iter != map_.end ())
> + {
> + map_.erase (iter);
> + return true;
> + }
> + return false;
> +}
> +
> /* Module profile is only used by LIPO. Here we simply ignore it. */
>
> static void
> @@ -2840,12 +3071,12 @@ afdo_indirect_call (gcall *stmt, const
> icall_target_map &map,
> }
> total *= afdo_count_scale;
> struct cgraph_node *direct_call = cgraph_node::get_for_asmname (
> - get_identifier (afdo_string_table->get_name (max_iter->first)));
> + get_identifier (afdo_string_table->get_symbol_name (max_iter->first)));
> if (direct_call == NULL)
> {
> if (dump_file)
> fprintf (dump_file, "Failed to find cgraph node for %s\n",
> - afdo_string_table->get_name (max_iter->first));
> + afdo_string_table->get_symbol_name (max_iter->first));
> return false;
> }
>
> --
> 2.44.0
>
