[13/14] Make cgraph_edge::uid really unique.

Message ID 77e86815fcbc8683a32403bdad1c68b023fac2f1.1526551813.git.mliska@suse.cz
State New
Headers show
Series
  • Finish transition of {symbol,call}_summary.
Related show

Commit Message

Martin Liška April 23, 2018, 11:50 a.m.
gcc/ChangeLog:

2018-04-24  Martin Liska  <mliska@suse.cz>

	* cgraph.c (symbol_table::create_edge): Always assign a new
	unique number.
	(symbol_table::free_edge): Do not recycle numbers.
	* cgraph.h (cgraph_edge::get): New method.
	* symbol-summary.h (symtab_removal): Use it.
	(symtab_duplication): Likewise.
	(call_summary::hashable_uid): Remove.
---
 gcc/cgraph.c         |  9 ++-------
 gcc/cgraph.h         | 14 +++++++++++---
 gcc/symbol-summary.h | 21 +++++++--------------
 3 files changed, 20 insertions(+), 24 deletions(-)

Comments

Jan Hubicka June 7, 2018, 12:19 p.m. | #1
> 

> gcc/ChangeLog:

> 

> 2018-04-24  Martin Liska  <mliska@suse.cz>

> 

> 	* cgraph.c (symbol_table::create_edge): Always assign a new

> 	unique number.

> 	(symbol_table::free_edge): Do not recycle numbers.

> 	* cgraph.h (cgraph_edge::get): New method.

> 	* symbol-summary.h (symtab_removal): Use it.

> 	(symtab_duplication): Likewise.

> 	(call_summary::hashable_uid): Remove.


OK.
Honza
Richard Biener June 7, 2018, 12:27 p.m. | #2
On Thu, Jun 7, 2018 at 2:19 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>

> >

> > gcc/ChangeLog:

> >

> > 2018-04-24  Martin Liska  <mliska@suse.cz>

> >

> >       * cgraph.c (symbol_table::create_edge): Always assign a new

> >       unique number.

> >       (symbol_table::free_edge): Do not recycle numbers.

> >       * cgraph.h (cgraph_edge::get): New method.

> >       * symbol-summary.h (symtab_removal): Use it.

> >       (symtab_duplication): Likewise.

> >       (call_summary::hashable_uid): Remove.


I think we should protect ourselves against overflow of the 'int'
counter (and make it unsigned?!).

Like with a simple gcc_assert (++edges_max_uid != 0); or so.

We throw away/recompute edges often enough so that we _might_
hit 2 billion edges, no?

Richard.

> OK.

> Honza
Jan Hubicka June 7, 2018, 12:36 p.m. | #3
> On Thu, Jun 7, 2018 at 2:19 PM Jan Hubicka <hubicka@ucw.cz> wrote:

> >

> > >

> > > gcc/ChangeLog:

> > >

> > > 2018-04-24  Martin Liska  <mliska@suse.cz>

> > >

> > >       * cgraph.c (symbol_table::create_edge): Always assign a new

> > >       unique number.

> > >       (symbol_table::free_edge): Do not recycle numbers.

> > >       * cgraph.h (cgraph_edge::get): New method.

> > >       * symbol-summary.h (symtab_removal): Use it.

> > >       (symtab_duplication): Likewise.

> > >       (call_summary::hashable_uid): Remove.

> 

> I think we should protect ourselves against overflow of the 'int'

> counter (and make it unsigned?!).

> 

> Like with a simple gcc_assert (++edges_max_uid != 0); or so.

> 

> We throw away/recompute edges often enough so that we _might_

> hit 2 billion edges, no?


We throw away those constant times for every function, but yes, overflow check
is quite likely good idea :)

Honza
Richard Biener June 7, 2018, 12:38 p.m. | #4
On Thu, Jun 7, 2018 at 2:36 PM Jan Hubicka <hubicka@ucw.cz> wrote:
>

> > On Thu, Jun 7, 2018 at 2:19 PM Jan Hubicka <hubicka@ucw.cz> wrote:

> > >

> > > >

> > > > gcc/ChangeLog:

> > > >

> > > > 2018-04-24  Martin Liska  <mliska@suse.cz>

> > > >

> > > >       * cgraph.c (symbol_table::create_edge): Always assign a new

> > > >       unique number.

> > > >       (symbol_table::free_edge): Do not recycle numbers.

> > > >       * cgraph.h (cgraph_edge::get): New method.

> > > >       * symbol-summary.h (symtab_removal): Use it.

> > > >       (symtab_duplication): Likewise.

> > > >       (call_summary::hashable_uid): Remove.

> >

> > I think we should protect ourselves against overflow of the 'int'

> > counter (and make it unsigned?!).

> >

> > Like with a simple gcc_assert (++edges_max_uid != 0); or so.

> >

> > We throw away/recompute edges often enough so that we _might_

> > hit 2 billion edges, no?

>

> We throw away those constant times for every function, but yes, overflow check

> is quite likely good idea :)


But you only need enough inline / speculative edges to make it blow up ;)
(the inline clones also might make the cgraph number explode in case you
manage to do some exponential inline cloning of empty functions...)

Richard.

> Honza

Patch

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index a24a5ffe521..572c775c14c 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -846,13 +846,11 @@  symbol_table::create_edge (cgraph_node *caller, cgraph_node *callee,
       free_edges = NEXT_FREE_EDGE (edge);
     }
   else
-    {
-      edge = ggc_alloc<cgraph_edge> ();
-      edge->uid = edges_max_uid++;
-    }
+    edge = ggc_alloc<cgraph_edge> ();
 
   edges_count++;
 
+  edge->m_uid = edges_max_uid++;
   edge->aux = NULL;
   edge->caller = caller;
   edge->callee = callee;
@@ -1006,14 +1004,11 @@  cgraph_edge::remove_caller (void)
 void
 symbol_table::free_edge (cgraph_edge *e)
 {
-  int uid = e->uid;
-
   if (e->indirect_info)
     ggc_free (e->indirect_info);
 
   /* Clear out the edge so we do not dangle pointers.  */
   memset (e, 0, sizeof (*e));
-  e->uid = uid;
   NEXT_FREE_EDGE (e) = free_edges;
   free_edges = e;
   edges_count--;
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 0e3b1a1785e..1966893343d 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -1633,6 +1633,7 @@  struct GTY(()) cgraph_indirect_call_info
 struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"),
 	    for_user)) cgraph_edge {
   friend class cgraph_node;
+  friend class symbol_table;
 
   /* Remove the edge in the cgraph.  */
   void remove (void);
@@ -1696,6 +1697,12 @@  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"),
   /* Return true if the call can be hot.  */
   bool maybe_hot_p (void);
 
+  /* Get unique identifier of the edge.  */
+  inline int get_uid ()
+  {
+    return m_uid;
+  }
+
   /* Rebuild cgraph edges for current function node.  This needs to be run after
      passes that don't update the cgraph.  */
   static unsigned int rebuild_edges (void);
@@ -1723,8 +1730,6 @@  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"),
   /* The stmt_uid of call_stmt.  This is used by LTO to recover the call_stmt
      when the function is serialized in.  */
   unsigned int lto_stmt_uid;
-  /* Unique id of the edge.  */
-  int uid;
   /* Whether this edge was made direct by indirect inlining.  */
   unsigned int indirect_inlining_edge : 1;
   /* Whether this edge describes an indirect call with an undetermined
@@ -1768,6 +1773,9 @@  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"),
   /* Expected frequency of executions within the function.  */
   sreal sreal_frequency ();
 private:
+  /* Unique id of the edge.  */
+  int m_uid;
+
   /* Remove the edge from the list of the callers of the callee.  */
   void remove_caller (void);
 
@@ -2018,7 +2026,7 @@  public:
   friend class cgraph_node;
   friend class cgraph_edge;
 
-  symbol_table (): cgraph_max_uid (1)
+  symbol_table (): cgraph_max_uid (1), edges_max_uid (1)
   {
   }
 
diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h
index 12e50201125..8c80f309372 100644
--- a/gcc/symbol-summary.h
+++ b/gcc/symbol-summary.h
@@ -375,19 +375,19 @@  public:
      If a summary for an edge does not exist, it will be created.  */
   T* get_create (cgraph_edge *edge)
   {
-    return get (hashable_uid (edge), true);
+    return get (edge->get_uid (), true);
   }
 
   /* Getter for summary callgraph edge pointer.  */
   T* get (cgraph_edge *edge)
   {
-    return get (hashable_uid (edge), false);
+    return get (edge->get_uid (), false);
   }
 
   /* Remove edge from summary.  */
   void remove (cgraph_edge *edge)
   {
-    int uid = hashable_uid (edge);
+    int uid = edge->get_uid ();
     T **v = m_map.get (uid);
     if (v)
       {
@@ -405,7 +405,7 @@  public:
   /* Return true if a summary for the given EDGE already exists.  */
   bool exists (cgraph_edge *edge)
   {
-    return m_map.get (hashable_uid (edge)) != NULL;
+    return m_map.get (edge->get_uid ()) != NULL;
   }
 
   /* Symbol removal hook that is registered to symbol table.  */
@@ -428,13 +428,6 @@  private:
   /* Getter for summary callgraph ID.  */
   T *get (int uid, bool lazy_insert);
 
-  /* Get a hashable uid of EDGE.  */
-  int hashable_uid (cgraph_edge *edge)
-  {
-    /* Edge uids start at zero which our hash_map does not like.  */
-    return edge->uid + 1;
-  }
-
   /* Main summary store, where summary ID is used as key.  */
   hash_map <map_hash, T *> m_map;
   /* Internal summary removal hook pointer.  */
@@ -511,7 +504,7 @@  call_summary<T *>::symtab_removal (cgraph_edge *edge, void *data)
 {
   call_summary *summary = (call_summary <T *> *) (data);
 
-  int h_uid = summary->hashable_uid (edge);
+  int h_uid = edge->get_uid ();
   T **v = summary->m_map.get (h_uid);
 
   if (v)
@@ -534,7 +527,7 @@  call_summary<T *>::symtab_duplication (cgraph_edge *edge1,
     edge1_summary = summary->get_create (edge1);
   else
     {
-      T **v = summary->m_map.get (summary->hashable_uid (edge1));
+      T **v = summary->m_map.get (edge1->get_uid ());
       if (v)
 	{
 	  /* This load is necessary, because we insert a new value!  */
@@ -545,7 +538,7 @@  call_summary<T *>::symtab_duplication (cgraph_edge *edge1,
   if (edge1_summary)
     {
       T *duplicate = summary->allocate_new ();
-      summary->m_map.put (summary->hashable_uid (edge2), duplicate);
+      summary->m_map.put (edge2->get_uid (), duplicate);
       summary->duplicate (edge1, edge2, edge1_summary, duplicate);
     }
 }