On 13/10/25 14:50, Jan Hubicka wrote:
External email: Use caution opening links or attachments
diff --git a/gcc/auto-profile.cc b/gcc/auto-profile.cc
index ce607a68d2e..189f98d683e 100644
--- a/gcc/auto-profile.cc
+++ b/gcc/auto-profile.cc
@@ -122,7 +122,7 @@ along with GCC; see the file COPYING3. If not see
*/
#define DEFAULT_AUTO_PROFILE_FILE "fbdata.afdo"
-#define AUTO_PROFILE_VERSION 2
+#define AUTO_PROFILE_VERSION 3
/* profile counts determined by AFDO smaller than afdo_hot_bb_threshold are
considered cols. */
@@ -170,7 +170,7 @@ struct decl_lineno
typedef auto_vec<decl_lineno, 20> inline_stack;
/* String array that stores function names. */
-typedef auto_vec<char *> string_vector;
+typedef auto_vec<const char *> string_vector;
/* Map from function name's index in string_table to target's
execution count. */
@@ -228,6 +228,23 @@ public:
/* For a given index, returns the string. */
const char *get_name (int index) const;
+ /* For a given index, returns the string. */
+ const char *get_filename (int index) const;
+
+ /* For a given function index, returns the index. */
+ int get_filename_idx (int index) const;
+
+ /* For a given function name, returns the index. */
+ int get_filename_idx (const char *name) const;
+
+ /* For a given filename, returns the index. */
+ int get_idx_for_file (const char *name) const;
These three functions looks quite confusing. We now have two tables -
symbol table and filename table, right? So we have two kind of indexes.
I think we want to be consistent how to call them, so perhaps filename
index and symbol index or so?
Hi Honza,
Yeah, my apologies for this, the naming isn't very good. The first two of these
return a filename index for a symbol and the third returns the filename index
for a file. I couldn't figure out a good set of names for this. I'll try and
get better naming in general for the next version of this patch.
+/* Find the matching function instance which has FUN_ID as its name. */
+
+function_instance *
+autofdo_source_profile::find_function_instance (unsigned fun_id,
+ unsigned file_name) const
+{
+ auto it = map_.find (file_name);
+ if (it == map_.end ())
+ return NULL;
+ auto fun = it->second.find (fun_id);
+ if (fun == it->second.end ())
+ return NULL;
+ return fun->second;
+}
Then this can be simplified to single lookup.
For the most part, yes, but there is a case in offline_external_functions ()
where
there is a symbol lookup with a filename from a different function_instance. I'm
not sure if this can be done with a single lookup?
+
+ /* Get the original name and file name index for a node. This will return
the
+ name from the current TU if there are multiple symbols that map to
+ NAME. */
+ std::pair<const char *, int> get_original_name (const char *name) const;
+
/* Read profile, return TRUE on success. */
bool read ();
@@ -238,14 +255,26 @@ public:
}
/* Add new name and return its index. */
- int add_name (char *);
+ int add_name (const char *string, int filename_idx);
+
+ /* Add new filename and return its index (returning the same if it already
+ * exists). */
+ int add_filename (const char *name);
/* Return cgraph node corresponding to given name index. */
cgraph_node *get_cgraph_node (int);
private:
typedef std::map<const char *, unsigned, string_compare> string_index_map;
+ typedef std::map<const char *, auto_vec<unsigned>, string_compare>
+ string_names_map;
+ typedef std::map<const char *, char *, string_compare> string_string_map;
string_vector vector_;
string_index_map map_;
+ string_vector filenames_;
+ string_index_map filenames_map_;
+ string_index_map name_filenames_map_;
+ string_string_map original_names_map_;
+ string_names_map clashing_names_;
};
/* Profile of a function instance:
@@ -468,6 +503,9 @@ private:
/* function_instance name index in the string_table. */
unsigned name_;
+ /* The file that the function_instance comes from. */
+ unsigned file_name_;
If I understand it correctly, the code is organized in a way that all
symbols are now stored as (symbol_name_index,filename_index) pairs.
Then it looks if there is a name clash using clashing_names_ table.
If so then it adjusts the lookup to also compare filanmes.
I think this can not work in case there are symbols that are public
in some units and internal in others. For exmaple when I have
a.c
int foo ()
{
}
b.c:
static int foo () { }
... use foo
c.c:
extern int foo () { }
... use foo
I think in this case we need to be sure that instance for a.c:foo is
saved with filename_index == 0 (or -1, since you seem to use it for
<unknown>) while internal instances such as b.c:foo is saved with
filename_index != 0.
For example indirect call in c.c may call both a.c:foo and b.c:foo so we
can not really distinguish them by notion of current file.
The assumption that I have made in this patch is that all symbols, regardless
of being internal or external, have filenames associated with them in the
DWARF info and in the auto-profile pass. This allows me to compare them
without having to worry about them being internal or external.
In that case both a.c:foo and b.c:foo will have unique filename indices
associated with them. This will also mean that c.c:foo will have a unique
filename index as well. As long as the compiler marks DECL_SOURCE_FILE with
the correct filename to look up for each call, this should work in theory.
I'm not sure how well this holds up in practice given that we have been
seeing cases where symbols have filenames completely missing and instances
are not getting marked correctly, so I think we do need to take a look at
this assumption again.
Even more interesting istuation is with comdat functions which are
originally public, but they may be duplicated and internalized by
different units. We also have clones of the same public comdat in
different units.
I believe in autofdo we can use DW_AT_external attribute to figure out
whether file_name index should be computed and then we can avoid need
for the filename clash heuristics.
There is a bit of an inefficiency in this patch where I annotate every
symbol regardless of being global or local, so this is quite a good
optimization for this.
Now dwarf2out produces DW_AT_external quite early, while autofdo pass is
run after early optimizations. So DECL_PUBLIC might have been cleared
and node might have been internalized at a time. We have
cgraph_node::make_local for that but it seems unused right now whle
multiple_target has its own ad-hoc copy of it. I think for now we can
ignore the problem; I will handle it incrementally.
There is similar logic in coverage_compute_profile_id which in fact
solves similar problem - it needs profile_id that is a hash of function
stable across while binary and also across different builds.
- /* For a given name index, returns the top-level function_instance. */
- function_instance *get_function_instance_by_name_index (int) const;
+ /* For a given name index (and original name), returns the top-level
+ * function_instance. */
Extra * also the second parameter (filename index) needs to be
mentioned.
+ function_instance *get_function_instance_by_name_index (int, int) const;
void add_function_instance (function_instance *);
@@ -555,9 +594,10 @@ public:
void offline_unrealized_inlines ();
private:
- /* Map from function_instance name index (in string_table) to
- function_instance. */
- typedef std::map<unsigned, function_instance *> name_function_instance_map;
+ /* Map from function_instance file index (in string_table) to name index (in
+ string_table) to function_instance. */
+ typedef std::map<unsigned, std::map<unsigned, function_instance *>>
+ name_function_instance_map;
Maps of maps seems quite heavy handled.
Can't this be a map from std::pair <unsigned, unsigned> or perhaps
invent a structure holding the pair of indices?
Yes, this can be better. This is just a bit of a prototype that I have
put together to get the patch functionally working.
+/* For a given function name, returns the index. */
+
+int
+string_table::get_filename_idx (const char *name) const
+{
+ auto it = name_filenames_map_.find (name);
+ if (it != name_filenames_map_.end () && it->second < filenames_.length ())
name_filename map is used to match public functions with the unit
defining them?
Yes.
+/* Get the original name for a node. This will return the name from the
+ current TU if there are multiple symbols that map to NAME. */
+
+std::pair<const char *, int>
+string_table::get_original_name (const char *name) const
+{
+ /* Check if the un-prefixed name differs from the actual name. */
+ auto stripped = original_names_map_.find (name);
+
+ /* The original name for the symbol is its name, i.e. there are no
+ suffixes. */
+ if (stripped == original_names_map_.end ())
+ return {name, get_filename_idx (name)};
Does the name always correspond to index returned and if so, who do you
need both?
I'm not sure I understand. In this case, yes, but its not always true.
+
+ /* Figure out if a clash exists. */
+ auto clash = clashing_names_.find (stripped->second);
+ gcc_assert (clash != clashing_names_.end ());
+
+ /* Try to find a function from the current TU. */
+ gcc_assert (clash->second.length () >= 1);
+ if (symtab_node *n
+ = cgraph_node::get_for_asmname (get_identifier (stripped->second));
+ n && is_a<cgraph_node *> (n))
+ for (cgraph_node *cn = dyn_cast<cgraph_node *> (n); cn;)
+ {
+ /* Check if there is a symbol in the current TU that has the same name
+ as in the GCOV. */
+ for (auto name : clash->second)
+ {
+ int filename_idx = get_filename_idx (name);
+ if (cn->definition && cn->has_gimple_body_p ()
+ && !strcmp (lbasename (DECL_SOURCE_FILE (cn->decl)),
+ get_filename (filename_idx)))
coverage_compute_profile_id also has parameter
-param=profile-func-internal-id=. Originally it hashed complete
DECL_SOURCE_FILE which has turned out to be problem for build
environments that use different directory for trianing run than for
final run. I think we can se same logic here. Have autofdo to stream
out full filenames and depending on the param either compare them in
full or only base filenames.
Thanks, I wasn't aware about this. One issue that I ran into with
DECL_SOURCE_FILE (which is why I started using lbasename ()) is that
it doesn't contain the full path to the source file. Looking at it now,
I also see lrealpath () in libiberty. Is that the right function to use
for this?
Honza
--
Regards,
Dhruv
