On Thu, 10 Jul 2025, Jan Hubicka wrote:
> Hi,
> to assign debug locations to corresponding statements auto-fdo uses
> discriminators. Documentation says that if given statement belongs to
> multiple
> basic blocks, the discrminator distinguishes them.
>
> Current implementation however only work fork statements that expands into a
> squence of gimple statements which forms a linear sequence, sicne it
> essentially tracks a current location and renews it each time new BB is found.
> This is commonly not true for C++ code as in:
>
> <bb 25> :
> [simulator/csimplemodule.cc:379:85] _40 =
> std::__cxx11::basic_string<char>::c_str ([simulator/csimplemodule.cc:379:85]
> &D.80680);
> [simulator/csimplemodule.cc:379:85 discrim 13] _41 =
> [simulator/csimplemodule.cc:379:85]
> &this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782;
> [simulator/csimplemodule.cc:379:85 discrim 13] _42 =
> &this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782;
> [simulator/csimplemodule.cc:377:45] _43 =
> this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782._vptr.cObject;
> [simulator/csimplemodule.cc:377:45] _44 = _43 + 40;
> [simulator/csimplemodule.cc:377:45] _45 =
> [simulator/csimplemodule.cc:377:45] *_44;
> [simulator/csimplemodule.cc:379:85] D.89001 = OBJ_TYPE_REF(_45;(const
> struct cObject)_42->5B) (_41);
>
> This is a fragment of code that is expanded from:
>
>
> 371 if (this!=simulation.getContextModule())
> 372 throw cRuntimeError("send()/sendDelayed() of module (%s)%s
> called in the context of "
> 373 "module (%s)%s: method called from the
> latter module "
> 374 "lacks Enter_Method() or
> Enter_Method_Silent()? "
> 375 "Also, if message to be sent is passed
> from that module, "
> 376 "you'll need to call take(msg) after
> Enter_Method() as well",
> 377 getClassName(), getFullPath().c_str(),
> 378
> simulation.getContextModule()->getClassName(),
> 379
> simulation.getContextModule()->getFullPath().c_str());
>
> Notice that 379:85 is interleaved by 377:45 and the pass does not assign new
> discriminator.
> With patch we get:
>
> <bb 25> :
> [simulator/csimplemodule.cc:379:85 discrim 7] _40 =
> std::__cxx11::basic_string<char>::c_str ([simulator/csimplemodule.cc:379:85]
> &D.80680);
> [simulator/csimplemodule.cc:379:85 discrim 8] _41 =
> [simulator/csimplemodule.cc:379:85]
> &this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782;
> [simulator/csimplemodule.cc:379:85 discrim 8] _42 =
> &this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782;
> [simulator/csimplemodule.cc:377:45 discrim 1] _43 =
> this->D.78503.D.78106.D.72008.D.68585.D.67935.D.67879.D.67782._vptr.cObject;
> [simulator/csimplemodule.cc:377:45 discrim 1] _44 = _43 + 40;
> [simulator/csimplemodule.cc:377:45 discrim 1] _45 =
> [simulator/csimplemodule.cc:377:45] *_44;
> [simulator/csimplemodule.cc:379:85 discrim 8] D.89001 =
> OBJ_TYPE_REF(_45;(const struct cObject)_42->5B) (_41);
>
> There are earlier statements with line number 379, so that is why there is
> discriminator 7 for the call.
> After that discriminator is increased. There are two reasons for it
> 1) AFDO requires every callsite to have unique lineno:discriminator pair
> 2) call may not terminate and htus the profile of first statement
> may be higher than the rest.
>
> Old pass also contained logic to skip debug statements. This is needed
> to discriminator at train time (with -g) and discriminators at feedback
> time (say -g0 -fauto-profile=...) are the same. However keeping debug
> statments with broken discriminators is not a good idea since we output
> them to the debug output and if AFDO tool picks these locations up they
> will be misplaced in basic blocks.
>
> Debug statements are naturally quite useful to track back the AFDO profiles
> and in meantime LLVM folks implemented something similar called pseudoprobe.
> I think it makes sense toenable debug statements with -fauto-profile even if
> debug info is off and make use of them as done in this patch.
>
> Sadly AFDO tool is quite broken and bulid around assumption that every
> address has at most one debug location assigned to it (i.e. debug info
> before debug statements were introduced). I have WIP patch fixing this.
> The fact that it ignores all but last location assigned to the address
> sort of mitigates problem with debug statements. If they are
> immediately suceeded by another location, the tool ignores them.
>
> Note that LLVM also has -fdebug-info-for-auto-profile (on by defualt it seems)
> that controls discriminator production and some other little bits. I wonder
> if
> we want to have something similar. Should it be -g<something> instead?
>
> Bootstrapped/regtested x86_64-linux, OK?
>
>
> I am including new code as diff did not good job.
>
> /* Auto-profile needs discriminator to distinguish statements with same line
> number (file name is ignored) which are in different basic block. This
> map keeps track of current discriminator for a given line number. */
> struct discrim_entry
> {
> /* ID of basic block we saw line number last time. */
> unsigned int bb_id;
> /* Discriminator we used. */
> unsigned int discrim;
> };
>
> /* Return updated LOC with discriminator for use in basic block BB_ID.
> MAP keeps track of current values. */
>
> location_t
> assign_discriminator (location_t loc, unsigned int bb_id,
> hash_map<int_hash <int64_t, -1, -2>, discrim_entry> &map)
> {
> bool existed;
> discrim_entry &e = map.get_or_insert (LOCATION_LINE (loc), &existed);
> gcc_checking_assert (!has_discriminator (loc));
> if (!existed)
> {
> e.bb_id = bb_id;
> e.discrim = 0;
> return loc;
> }
> if (e.bb_id != bb_id)
> {
> e.bb_id = bb_id;
> e.discrim++;
> }
> if (e.discrim)
> return location_with_discriminator (loc, e.discrim);
> return loc;
> }
>
> /* Assign discriminators to statement locations. */
>
> static void
> assign_discriminators (void)
> {
> hash_map<int_hash <int64_t, -1, -2>, discrim_entry> map (13);
> unsigned int bb_id = 0;
> basic_block bb;
> FOR_EACH_BB_FN (bb, cfun)
> {
> location_t prev_loc = UNKNOWN_LOCATION, prev_replacement =
> UNKNOWN_LOCATION;
> /* Traverse the basic block, if two function calls within a basic block
> are mapped to the same line, assign a new discriminator because a call
> stmt could be a split point of a basic block. */
> for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> !gsi_end_p (gsi); gsi_next (&gsi))
> {
> gimple *stmt = gsi_stmt (gsi);
> location_t loc = gimple_location (stmt);
> if (loc == UNKNOWN_LOCATION)
> continue;
> if (loc == prev_loc)
> gimple_set_location (stmt, prev_replacement);
> else
> {
> prev_loc = loc;
> prev_replacement = assign_discriminator (loc, bb_id, map);
So with this the discriminator we assign might depend on whether
we have debug stmts or not. We output them only to debug info, so
it should in principle not cause compare-debug issues, right? And
we don't use discriminators to affect code generation (hopefully).
The change looks reasonable otherwise.
Richard.
> gimple_set_location (stmt, prev_replacement);
> }
> /* Break basic blocks after each call. AFDO requires each
> call site to have unique discriminator.
> More correctly, we can break after each statement that can possibly
> terinate execution of the basic block, but for auto-profile this
> precision is probably not useful. */
> if (gimple_code (stmt) == GIMPLE_CALL)
> {
> prev_loc = UNKNOWN_LOCATION;
> bb_id++;
> }
> }
> /* IF basic block has multiple sucessors, consdier every edge as a
> separate
> block. */
> if (!single_succ_p (bb))
> bb_id++;
> for (edge e : bb->succs)
> if (e->goto_locus != UNKNOWN_LOCATION)
> {
> e->goto_locus = assign_discriminator (e->goto_locus, bb_id, map);
> bb_id++;
> }
> bb_id++;
> }
> }
>
> gcc/ChangeLog:
>
> * opts.cc (finish_options): Enable debug_nonbind_markers_p for
> auto-profile.
> * tree-cfg.cc (struct locus_discrim_map): Remove.
> (struct locus_discrim_hasher): Remove.
> (locus_discrim_hasher::hash): Remove.
> (locus_discrim_hasher::equal): Remove.
> (first_non_label_nondebug_stmt): Remove.
> (build_gimple_cfg): Do not allocate discriminator tables.
> (next_discriminator_for_locus): Remove.
> (same_line_p): Remove.
> (struct discrim_entry): New structure.
> (assign_discriminator): Rewrite.
> (assign_discriminators): Rewrite.
>
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 6ca1ec7e865..60ad633b7ff 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -1411,11 +1411,14 @@ finish_options (struct gcc_options *opts, struct
> gcc_options *opts_set,
> opts->x_debug_info_level = DINFO_LEVEL_NONE;
> }
>
> + /* Also enable markers with -fauto-profile even when debug info is
> disabled,
> + so we assign same discriminators and can read back the profile info. */
> if (!opts_set->x_debug_nonbind_markers_p)
> opts->x_debug_nonbind_markers_p
> = (opts->x_optimize
> - && opts->x_debug_info_level >= DINFO_LEVEL_NORMAL
> - && (dwarf_debuginfo_p (opts) || codeview_debuginfo_p ())
> + && ((opts->x_debug_info_level >= DINFO_LEVEL_NORMAL
> + && (dwarf_debuginfo_p (opts) || codeview_debuginfo_p ()))
> + || opts->x_flag_auto_profile)
> && !(opts->x_flag_selective_scheduling
> || opts->x_flag_selective_scheduling2));
>
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index 9a5479a2d38..3d23f178da7 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -114,43 +114,6 @@ struct replace_decls_d
> tree to_context;
> };
>
> -/* Hash table to store last discriminator assigned for each locus. */
> -struct locus_discrim_map
> -{
> - int location_line;
> - int discriminator;
> -};
> -
> -/* Hashtable helpers. */
> -
> -struct locus_discrim_hasher : free_ptr_hash <locus_discrim_map>
> -{
> - static inline hashval_t hash (const locus_discrim_map *);
> - static inline bool equal (const locus_discrim_map *,
> - const locus_discrim_map *);
> -};
> -
> -/* Trivial hash function for a location_t. ITEM is a pointer to
> - a hash table entry that maps a location_t to a discriminator. */
> -
> -inline hashval_t
> -locus_discrim_hasher::hash (const locus_discrim_map *item)
> -{
> - return item->location_line;
> -}
> -
> -/* Equality function for the locus-to-discriminator map. A and B
> - point to the two hash table entries to compare. */
> -
> -inline bool
> -locus_discrim_hasher::equal (const locus_discrim_map *a,
> - const locus_discrim_map *b)
> -{
> - return a->location_line == b->location_line;
> -}
> -
> -static hash_table<locus_discrim_hasher> *discriminator_per_locus;
> -
> /* Basic blocks and flowgraphs. */
> static void make_blocks (gimple_seq);
>
> @@ -168,7 +131,6 @@ static edge gimple_try_redirect_by_replacing_jump (edge,
> basic_block);
> static inline bool stmt_starts_bb_p (gimple *, gimple *);
> static bool gimple_verify_flow_info (void);
> static void gimple_make_forwarder_block (edge);
> -static gimple *first_non_label_nondebug_stmt (basic_block);
> static bool verify_gimple_transaction (gtransaction *);
> static bool call_can_make_abnormal_goto (gimple *);
>
> @@ -247,12 +209,9 @@ build_gimple_cfg (gimple_seq seq)
> group_case_labels ();
>
> /* Create the edges of the flowgraph. */
> - discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
> make_edges ();
> assign_discriminators ();
> cleanup_dead_labels ();
> - delete discriminator_per_locus;
> - discriminator_per_locus = NULL;
> }
>
> /* Look for ANNOTATE calls with loop annotation kind in BB; if found, remove
> @@ -1120,77 +1079,41 @@ gimple_find_sub_bbs (gimple_seq seq,
> gimple_stmt_iterator *gsi)
> return true;
> }
>
> -/* Find the next available discriminator value for LOCUS. The
> - discriminator distinguishes among several basic blocks that
> - share a common locus, allowing for more accurate sample-based
> - profiling. */
> -
> -static int
> -next_discriminator_for_locus (int line)
> +/* Auto-profile needs discriminator to distinguish statements with same line
> + number (file name is ignored) which are in different basic block. This
> + map keeps track of current discriminator for a given line number. */
> +struct discrim_entry
> {
> - struct locus_discrim_map item;
> - struct locus_discrim_map **slot;
> -
> - item.location_line = line;
> - item.discriminator = 0;
> - slot = discriminator_per_locus->find_slot_with_hash (&item, line, INSERT);
> - gcc_assert (slot);
> - if (*slot == HTAB_EMPTY_ENTRY)
> - {
> - *slot = XNEW (struct locus_discrim_map);
> - gcc_assert (*slot);
> - (*slot)->location_line = line;
> - (*slot)->discriminator = 0;
> - }
> - (*slot)->discriminator++;
> - return (*slot)->discriminator;
> -}
> -
> -/* Return TRUE if LOCUS1 and LOCUS2 refer to the same source line. */
> -
> -static bool
> -same_line_p (location_t locus1, expanded_location *from, location_t locus2)
> -{
> - expanded_location to;
> -
> - if (locus1 == locus2)
> - return true;
> -
> - to = expand_location (locus2);
> -
> - if (from->line != to.line)
> - return false;
> - if (from->file == to.file)
> - return true;
> - return (from->file != NULL
> - && to.file != NULL
> - && filename_cmp (from->file, to.file) == 0);
> -}
> + /* ID of basic block we saw line number last time. */
> + unsigned int bb_id;
> + /* Discriminator we used. */
> + unsigned int discrim;
> +};
>
> -/* Assign a unique discriminator value to all statements in block bb that
> - have the same line number as locus. */
> +/* Return updated LOC with discriminator for use in basic block BB_ID.
> + MAP keeps track of current values. */
>
> -static void
> -assign_discriminator (location_t locus, basic_block bb)
> +location_t
> +assign_discriminator (location_t loc, unsigned int bb_id,
> + hash_map<int_hash <int64_t, -1, -2>, discrim_entry> &map)
> {
> - gimple_stmt_iterator gsi;
> - int discriminator;
> -
> - if (locus == UNKNOWN_LOCATION)
> - return;
> -
> - expanded_location locus_e = expand_location (locus);
> -
> - discriminator = next_discriminator_for_locus (locus_e.line);
> -
> - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + bool existed;
> + discrim_entry &e = map.get_or_insert (LOCATION_LINE (loc), &existed);
> + gcc_checking_assert (!has_discriminator (loc));
> + if (!existed)
> {
> - gimple *stmt = gsi_stmt (gsi);
> - location_t stmt_locus = gimple_location (stmt);
> - if (same_line_p (locus, &locus_e, stmt_locus))
> - gimple_set_location (stmt,
> - location_with_discriminator (stmt_locus, discriminator));
> + e.bb_id = bb_id;
> + e.discrim = 0;
> + return loc;
> + }
> + if (e.bb_id != bb_id)
> + {
> + e.bb_id = bb_id;
> + e.discrim++;
> }
> + if (e.discrim)
> + return location_with_discriminator (loc, e.discrim);
> + return loc;
> }
>
> /* Assign discriminators to statement locations. */
> @@ -1198,92 +1121,54 @@ assign_discriminator (location_t locus, basic_block
> bb)
> static void
> assign_discriminators (void)
> {
> + hash_map<int_hash <int64_t, -1, -2>, discrim_entry> map (13);
> + unsigned int bb_id = 0;
> basic_block bb;
> -
> FOR_EACH_BB_FN (bb, cfun)
> {
> - edge e;
> - edge_iterator ei;
> - gimple_stmt_iterator gsi;
> - location_t curr_locus = UNKNOWN_LOCATION;
> - expanded_location curr_locus_e = {};
> - int curr_discr = 0;
> -
> + location_t prev_loc = UNKNOWN_LOCATION, prev_replacement =
> UNKNOWN_LOCATION;
> /* Traverse the basic block, if two function calls within a basic block
> are mapped to the same line, assign a new discriminator because a call
> stmt could be a split point of a basic block. */
> - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + for (gimple_stmt_iterator gsi = gsi_start_bb (bb);
> + !gsi_end_p (gsi); gsi_next (&gsi))
> {
> gimple *stmt = gsi_stmt (gsi);
> -
> - /* Don't allow debug stmts to affect discriminators, but
> - allow them to take discriminators when they're on the
> - same line as the preceding nondebug stmt. */
> - if (is_gimple_debug (stmt))
> - {
> - if (curr_locus != UNKNOWN_LOCATION
> - && same_line_p (curr_locus, &curr_locus_e,
> - gimple_location (stmt)))
> - {
> - location_t loc = gimple_location (stmt);
> - location_t dloc = location_with_discriminator (loc,
> - curr_discr);
> - gimple_set_location (stmt, dloc);
> - }
> - continue;
> - }
> - if (curr_locus == UNKNOWN_LOCATION)
> - {
> - curr_locus = gimple_location (stmt);
> - curr_locus_e = expand_location (curr_locus);
> - }
> - else if (!same_line_p (curr_locus, &curr_locus_e, gimple_location
> (stmt)))
> - {
> - curr_locus = gimple_location (stmt);
> - curr_locus_e = expand_location (curr_locus);
> - curr_discr = 0;
> - }
> - else if (curr_discr != 0)
> + location_t loc = gimple_location (stmt);
> + if (loc == UNKNOWN_LOCATION)
> + continue;
> + if (loc == prev_loc)
> + gimple_set_location (stmt, prev_replacement);
> + else
> {
> - location_t loc = gimple_location (stmt);
> - location_t dloc = location_with_discriminator (loc, curr_discr);
> - gimple_set_location (stmt, dloc);
> + prev_loc = loc;
> + prev_replacement = assign_discriminator (loc, bb_id, map);
> + gimple_set_location (stmt, prev_replacement);
> }
> - /* Allocate a new discriminator for CALL stmt. */
> + /* Break basic blocks after each call. This is requires so each
> + call site has unque discriminator.
> + More correctly, we can break after each statement that can possibly
> + terinate execution of the basic block, but for auto-profile this
> + precision is probably not useful. */
> if (gimple_code (stmt) == GIMPLE_CALL)
> - curr_discr = next_discriminator_for_locus (curr_locus_e.line);
> - }
> -
> - gimple *last = last_nondebug_stmt (bb);
> - location_t locus = last ? gimple_location (last) : UNKNOWN_LOCATION;
> - if (locus == UNKNOWN_LOCATION)
> - continue;
> -
> - expanded_location locus_e = expand_location (locus);
> -
> - FOR_EACH_EDGE (e, ei, bb->succs)
> - {
> - gimple *first = first_non_label_nondebug_stmt (e->dest);
> - gimple *last = last_nondebug_stmt (e->dest);
> -
> - gimple *stmt_on_same_line = NULL;
> - if (first && same_line_p (locus, &locus_e,
> - gimple_location (first)))
> - stmt_on_same_line = first;
> - else if (last && same_line_p (locus, &locus_e,
> - gimple_location (last)))
> - stmt_on_same_line = last;
> -
> - if (stmt_on_same_line)
> {
> - if (has_discriminator (gimple_location (stmt_on_same_line))
> - && !has_discriminator (locus))
> - assign_discriminator (locus, bb);
> - else
> - assign_discriminator (locus, e->dest);
> + prev_loc = UNKNOWN_LOCATION;
> + bb_id++;
> }
> }
> + /* IF basic block has multiple sucessors, consdier every edge as a
> separate
> + block. */
> + if (!single_succ_p (bb))
> + bb_id++;
> + for (edge e : bb->succs)
> + if (e->goto_locus != UNKNOWN_LOCATION)
> + {
> + e->goto_locus = assign_discriminator (e->goto_locus, bb_id, map);
> + bb_id++;
> + }
> + bb_id++;
> }
> +
> }
>
> /* Create the edges for a GIMPLE_COND starting at block BB. */
> @@ -2948,16 +2833,6 @@ first_stmt (basic_block bb)
> return stmt;
> }
>
> -/* Return the first non-label/non-debug statement in basic block BB. */
> -
> -static gimple *
> -first_non_label_nondebug_stmt (basic_block bb)
> -{
> - gimple_stmt_iterator i;
> - i = gsi_start_nondebug_after_labels_bb (bb);
> - return !gsi_end_p (i) ? gsi_stmt (i) : NULL;
> -}
> -
> /* Return the last statement in basic block BB. */
>
> gimple *
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
