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);
              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 *

Reply via email to