On Fri, Jul 11, 2025 at 2:26 AM Richard Biener <rguent...@suse.de> wrote:
>
> 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).
yes in fact the discriminator assigning code required a few fixes
before for compare debug issues.
r15-8075-gc5ca45b8069229
r15-5535-g81c29232b6f362
r14-5257-g61d2b4746300a6
Maybe it would be useful to run -fcompare-debug with GCC's testsuite
to see if there was any new fall out with the rewritten one?
Thanks,
Andrew Pinski
>
> 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)
