[PATCH] Reorder fields in struct cgraph_edge et al

[PATCH] Reorder fields in struct cgraph_edge et al

[Martin Jambor]

Hi,

I have noticed that struct cgraph_edge is currently misaligned by one
bit.  This patch fixes that by making loop_nest an unsigned short, and
so leaving plenty of bits available for future flags.  When doing so,
I also reordered the fields according to their size which should also
decrease the size of the structure.  Then I did the same thing with a
very similar structure in ipa-prop.h, namely param_call_note.
Finally, because I have noticed that loop_nest is not set, I fixed
that.

I have bootstrapped and tested this on x86_64-linux.  I seek
permission to commit this to trunk despite stage 4 looming on the
horizon.  (Fortunately I do not have any more patches like this :-)

Thanks,

Martin



2009-11-30  Martin Jambor  <mjambor@suse.cz>

        * cgraph.h (GTY): Reorder fields.  Make loop_nest unsigned short int.
	* ipa-prop.h (struct ipa_param_call_note): Likewise.
	* ipa-prop.c (ipa_note_param_call): Set note->loop_nest.

Index: icln/gcc/cgraph.h
===================================================================
--- icln.orig/gcc/cgraph.h
+++ icln/gcc/cgraph.h
@@ -310,6 +310,8 @@ typedef enum {
 } cgraph_inline_failed_t;
 
 struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
+  /* Expected number of executions: calculated in profile.c.  */
+  gcov_type count;
   struct cgraph_node *caller;
   struct cgraph_node *callee;
   struct cgraph_edge *prev_caller;
@@ -317,29 +319,27 @@ struct GTY((chain_next ("%h.next_caller"
   struct cgraph_edge *prev_callee;
   struct cgraph_edge *next_callee;
   gimple call_stmt;
-  /* The stmt_uid of this call stmt.  This is used by LTO to recover
-     the call_stmt when the function is serialized in.  */
-  unsigned int lto_stmt_uid;
   PTR GTY ((skip (""))) aux;
   /* When equal to CIF_OK, inline this call.  Otherwise, points to the
      explanation why function was not inlined.  */
   cgraph_inline_failed_t inline_failed;
-  /* Expected number of executions: calculated in profile.c.  */
-  gcov_type count;
+  /* 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;
   /* Expected frequency of executions within the function.
      When set to CGRAPH_FREQ_BASE, the edge is expected to be called once
      per function call.  The range is 0 to CGRAPH_FREQ_MAX.  */
   int frequency;
+  /* Unique id of the edge.  */
+  int uid;
   /* Depth of loop nest, 1 means no loop nest.  */
-  unsigned int loop_nest : 30;
+  unsigned short int loop_nest;
   /* Whether this edge describes a call that was originally indirect.  */
   unsigned int indirect_call : 1;
   /* True if the corresponding CALL stmt cannot be inlined.  */
   unsigned int call_stmt_cannot_inline_p : 1;
   /* Can this call throw externally?  */
   unsigned int can_throw_external : 1;
-  /* Unique id of the edge.  */
-  int uid;
 };
 
 #define CGRAPH_FREQ_BASE 1000
Index: icln/gcc/ipa-prop.c
===================================================================
--- icln.orig/gcc/ipa-prop.c
+++ icln/gcc/ipa-prop.c
@@ -754,6 +754,7 @@ ipa_note_param_call (struct ipa_node_par
   note->lto_stmt_uid = gimple_uid (stmt);
   note->count = bb->count;
   note->frequency = compute_call_stmt_bb_frequency (current_function_decl, bb);
+  note->loop_nest = bb->loop_depth;
 
   note->next = info->param_calls;
   info->param_calls = note;
Index: icln/gcc/ipa-prop.h
===================================================================
--- icln.orig/gcc/ipa-prop.h
+++ icln/gcc/ipa-prop.h
@@ -139,6 +139,8 @@ struct ipcp_lattice
    are linked in a list.  */
 struct ipa_param_call_note
 {
+  /* Expected number of executions: calculated in profile.c.  */
+  gcov_type count;
   /* Linked list's next */
   struct ipa_param_call_note *next;
   /* Statement that contains the call to the parameter above.  */
@@ -147,13 +149,11 @@ struct ipa_param_call_note
   unsigned int lto_stmt_uid;
   /* Index of the parameter that is called.  */
   int formal_id;
-  /* Expected number of executions: calculated in profile.c.  */
-  gcov_type count;
   /* Expected frequency of executions within the function. see cgraph_edge in
      cgraph.h for more on this. */
   int frequency;
   /* Depth of loop nest, 1 means no loop nest.  */
-  int loop_nest;
+  unsigned short int loop_nest;
   /* Set when we have already found the target to be a compile time constant
      and turned this into an edge or when the note was found unusable for some
      reason.  */

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Jan Hubicka]

> Hi,
> 
> I have noticed that struct cgraph_edge is currently misaligned by one
> bit.  This patch fixes that by making loop_nest an unsigned short, and
> so leaving plenty of bits available for future flags.  When doing so,
> I also reordered the fields according to their size which should also
> decrease the size of the structure.  Then I did the same thing with a
> very similar structure in ipa-prop.h, namely param_call_note.
> Finally, because I have noticed that loop_nest is not set, I fixed
> that.
> 
> I have bootstrapped and tested this on x86_64-linux.  I seek
> permission to commit this to trunk despite stage 4 looming on the
> horizon.  (Fortunately I do not have any more patches like this :-)

This is OK.  Thanks!
Honza

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Richard Guenther]

On Mon, 30 Nov 2009, Martin Jambor wrote:

> Hi,
> 
> I have noticed that struct cgraph_edge is currently misaligned by one
> bit.  This patch fixes that by making loop_nest an unsigned short, and
> so leaving plenty of bits available for future flags.  When doing so,
> I also reordered the fields according to their size which should also
> decrease the size of the structure.  Then I did the same thing with a
> very similar structure in ipa-prop.h, namely param_call_note.
> Finally, because I have noticed that loop_nest is not set, I fixed
> that.
> 
> I have bootstrapped and tested this on x86_64-linux.  I seek
> permission to commit this to trunk despite stage 4 looming on the
> horizon.  (Fortunately I do not have any more patches like this :-)
> 
> Thanks,
> 
> Martin
> 
> 
> 
> 2009-11-30  Martin Jambor  <mjambor@suse.cz>
> 
>         * cgraph.h (GTY): Reorder fields.  Make loop_nest unsigned short int.
> 	* ipa-prop.h (struct ipa_param_call_note): Likewise.
> 	* ipa-prop.c (ipa_note_param_call): Set note->loop_nest.
> 
> Index: icln/gcc/cgraph.h
> ===================================================================
> --- icln.orig/gcc/cgraph.h
> +++ icln/gcc/cgraph.h
> @@ -310,6 +310,8 @@ typedef enum {
>  } cgraph_inline_failed_t;
>  
>  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
> +  /* Expected number of executions: calculated in profile.c.  */
> +  gcov_type count;

you now have introduced 32bit padding here ...

>    struct cgraph_node *caller;
>    struct cgraph_node *callee;
>    struct cgraph_edge *prev_caller;
> @@ -317,29 +319,27 @@ struct GTY((chain_next ("%h.next_caller"
>    struct cgraph_edge *prev_callee;
>    struct cgraph_edge *next_callee;
>    gimple call_stmt;
> -  /* The stmt_uid of this call stmt.  This is used by LTO to recover
> -     the call_stmt when the function is serialized in.  */
> -  unsigned int lto_stmt_uid;
>    PTR GTY ((skip (""))) aux;
>    /* When equal to CIF_OK, inline this call.  Otherwise, points to the
>       explanation why function was not inlined.  */
>    cgraph_inline_failed_t inline_failed;
> -  /* Expected number of executions: calculated in profile.c.  */
> -  gcov_type count;
> +  /* 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;

moving this is ok, please leave count where it is

>    /* Expected frequency of executions within the function.
>       When set to CGRAPH_FREQ_BASE, the edge is expected to be called once
>       per function call.  The range is 0 to CGRAPH_FREQ_MAX.  */
>    int frequency;
> +  /* Unique id of the edge.  */
> +  int uid;
>    /* Depth of loop nest, 1 means no loop nest.  */
> -  unsigned int loop_nest : 30;
> +  unsigned short int loop_nest;
>    /* Whether this edge describes a call that was originally indirect.  */
>    unsigned int indirect_call : 1;
>    /* True if the corresponding CALL stmt cannot be inlined.  */
>    unsigned int call_stmt_cannot_inline_p : 1;
>    /* Can this call throw externally?  */
>    unsigned int can_throw_external : 1;
> -  /* Unique id of the edge.  */
> -  int uid;
>  };
>  
>  #define CGRAPH_FREQ_BASE 1000
> Index: icln/gcc/ipa-prop.c
> ===================================================================
> --- icln.orig/gcc/ipa-prop.c
> +++ icln/gcc/ipa-prop.c
> @@ -754,6 +754,7 @@ ipa_note_param_call (struct ipa_node_par
>    note->lto_stmt_uid = gimple_uid (stmt);
>    note->count = bb->count;
>    note->frequency = compute_call_stmt_bb_frequency (current_function_decl, bb);
> +  note->loop_nest = bb->loop_depth;
>  
>    note->next = info->param_calls;
>    info->param_calls = note;
> Index: icln/gcc/ipa-prop.h
> ===================================================================
> --- icln.orig/gcc/ipa-prop.h
> +++ icln/gcc/ipa-prop.h
> @@ -139,6 +139,8 @@ struct ipcp_lattice
>     are linked in a list.  */
>  struct ipa_param_call_note
>  {
> +  /* Expected number of executions: calculated in profile.c.  */
> +  gcov_type count;

Likewise.  gcov_type is 32bit.

Ok with that chagnes.

Richard.

>    /* Linked list's next */
>    struct ipa_param_call_note *next;
>    /* Statement that contains the call to the parameter above.  */
> @@ -147,13 +149,11 @@ struct ipa_param_call_note
>    unsigned int lto_stmt_uid;
>    /* Index of the parameter that is called.  */
>    int formal_id;
> -  /* Expected number of executions: calculated in profile.c.  */
> -  gcov_type count;
>    /* Expected frequency of executions within the function. see cgraph_edge in
>       cgraph.h for more on this. */
>    int frequency;
>    /* Depth of loop nest, 1 means no loop nest.  */
> -  int loop_nest;
> +  unsigned short int loop_nest;
>    /* Set when we have already found the target to be a compile time constant
>       and turned this into an edge or when the note was found unusable for some
>       reason.  */
> 
> 

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Martin Jambor]

Hi,

On Tue, Dec 01, 2009 at 10:46:33AM +0100, Richard Guenther wrote:
> On Mon, 30 Nov 2009, Martin Jambor wrote:
> 

...

> > 2009-11-30  Martin Jambor  <mjambor@suse.cz>
> > 
> >         * cgraph.h (GTY): Reorder fields.  Make loop_nest unsigned short int.
> > 	* ipa-prop.h (struct ipa_param_call_note): Likewise.
> > 	* ipa-prop.c (ipa_note_param_call): Set note->loop_nest.
> > 
> > Index: icln/gcc/cgraph.h
> > ===================================================================
> > --- icln.orig/gcc/cgraph.h
> > +++ icln/gcc/cgraph.h
> > @@ -310,6 +310,8 @@ typedef enum {
> >  } cgraph_inline_failed_t;
> >  
> >  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
> > +  /* Expected number of executions: calculated in profile.c.  */
> > +  gcov_type count;
> 
> you now have introduced 32bit padding here ...
> 

...

> 
> moving this is ok, please leave count where it is
> 

...

> > +  /* Expected number of executions: calculated in profile.c.  */
> > +  gcov_type count;
> 
> Likewise.  gcov_type is 32bit.
> 

No, even on i386 gcov_type is 64 bit.  I have just double checked.
I have committed the patch as it was per Honza's approval.

Martin


> Ok with that chagnes.
> 
> Richard.
> 
> >    /* Linked list's next */
> >    struct ipa_param_call_note *next;
> >    /* Statement that contains the call to the parameter above.  */
> > @@ -147,13 +149,11 @@ struct ipa_param_call_note
> >    unsigned int lto_stmt_uid;
> >    /* Index of the parameter that is called.  */
> >    int formal_id;
> > -  /* Expected number of executions: calculated in profile.c.  */
> > -  gcov_type count;
> >    /* Expected frequency of executions within the function. see cgraph_edge in
> >       cgraph.h for more on this. */
> >    int frequency;
> >    /* Depth of loop nest, 1 means no loop nest.  */
> > -  int loop_nest;
> > +  unsigned short int loop_nest;
> >    /* Set when we have already found the target to be a compile time constant
> >       and turned this into an edge or when the note was found unusable for some
> >       reason.  */
> > 
> > 

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Richard Guenther]

On Tue, 1 Dec 2009, Martin Jambor wrote:

> Hi,
> 
> On Tue, Dec 01, 2009 at 10:46:33AM +0100, Richard Guenther wrote:
> > On Mon, 30 Nov 2009, Martin Jambor wrote:
> > 
> 
> ...
> 
> > > 2009-11-30  Martin Jambor  <mjambor@suse.cz>
> > > 
> > >         * cgraph.h (GTY): Reorder fields.  Make loop_nest unsigned short int.
> > > 	* ipa-prop.h (struct ipa_param_call_note): Likewise.
> > > 	* ipa-prop.c (ipa_note_param_call): Set note->loop_nest.
> > > 
> > > Index: icln/gcc/cgraph.h
> > > ===================================================================
> > > --- icln.orig/gcc/cgraph.h
> > > +++ icln/gcc/cgraph.h
> > > @@ -310,6 +310,8 @@ typedef enum {
> > >  } cgraph_inline_failed_t;
> > >  
> > >  struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
> > > +  /* Expected number of executions: calculated in profile.c.  */
> > > +  gcov_type count;
> > 
> > you now have introduced 32bit padding here ...
> > 
> 
> No, even on i386 gcov_type is 64 bit.  I have just double checked.
> I have committed the patch as it was per Honza's approval.

But what's the point moving it first, even before the next pointer?
The next pointer is accessed during traversing, possibly quite more
frequent than count.

Richard.

> 
> Martin
> 
> 
> > Ok with that chagnes.
> > 
> > Richard.
> > 
> > >    /* Linked list's next */
> > >    struct ipa_param_call_note *next;
> > >    /* Statement that contains the call to the parameter above.  */
> > > @@ -147,13 +149,11 @@ struct ipa_param_call_note
> > >    unsigned int lto_stmt_uid;
> > >    /* Index of the parameter that is called.  */
> > >    int formal_id;
> > > -  /* Expected number of executions: calculated in profile.c.  */
> > > -  gcov_type count;
> > >    /* Expected frequency of executions within the function. see cgraph_edge in
> > >       cgraph.h for more on this. */
> > >    int frequency;
> > >    /* Depth of loop nest, 1 means no loop nest.  */
> > > -  int loop_nest;
> > > +  unsigned short int loop_nest;
> > >    /* Set when we have already found the target to be a compile time constant
> > >       and turned this into an edge or when the note was found unusable for some
> > >       reason.  */
> > > 
> > > 
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Martin Jambor]

On Tue, Dec 01, 2009 at 12:04:58PM +0100, Richard Guenther wrote:
> On Tue, 1 Dec 2009, Martin Jambor wrote:
> 
> > Hi,
> > 
> > On Tue, Dec 01, 2009 at 10:46:33AM +0100, Richard Guenther wrote:
> > > 
> > > you now have introduced 32bit padding here ...
> > > 
> > 
> > No, even on i386 gcov_type is 64 bit.  I have just double checked.
> > I have committed the patch as it was per Honza's approval.
> 
> But what's the point moving it first, even before the next pointer?
> The next pointer is accessed during traversing, possibly quite more
> frequent than count.
> 

Well, after I saw how misaligned everything was (and so how little we,
including myself, sometimes care about this when we add stuff), I
thought that reordering the fields according to their size was the
safest way of preventing someone from inadvertently reintroducing the
padding.  Even more so because nobody seems to really remember how big
gcov_type actually is (I also had to ask Honza, probably not for the
first time).

Having said that, putting it after next_callee with a comment about
this is probably also rather safe (as opposed to anything lower than
that).  I can do that if you want.

Martin


> Richard.
> 
> > 
> > Martin
> > 
> > 
> > > Ok with that chagnes.
> > > 
> > > Richard.
> > > 
> > > >    /* Linked list's next */
> > > >    struct ipa_param_call_note *next;
> > > >    /* Statement that contains the call to the parameter above.  */
> > > > @@ -147,13 +149,11 @@ struct ipa_param_call_note
> > > >    unsigned int lto_stmt_uid;
> > > >    /* Index of the parameter that is called.  */
> > > >    int formal_id;
> > > > -  /* Expected number of executions: calculated in profile.c.  */
> > > > -  gcov_type count;
> > > >    /* Expected frequency of executions within the function. see cgraph_edge in
> > > >       cgraph.h for more on this. */
> > > >    int frequency;
> > > >    /* Depth of loop nest, 1 means no loop nest.  */
> > > > -  int loop_nest;
> > > > +  unsigned short int loop_nest;
> > > >    /* Set when we have already found the target to be a compile time constant
> > > >       and turned this into an edge or when the note was found unusable for some
> > > >       reason.  */
> > > > 
> > > > 
> > 
> > 
> 
> -- 
> Richard Guenther <rguenther@suse.de>
> Novell / SUSE Labs
> SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Richard Guenther]

On Tue, 1 Dec 2009, Martin Jambor wrote:

> On Tue, Dec 01, 2009 at 12:04:58PM +0100, Richard Guenther wrote:
> > On Tue, 1 Dec 2009, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > On Tue, Dec 01, 2009 at 10:46:33AM +0100, Richard Guenther wrote:
> > > > 
> > > > you now have introduced 32bit padding here ...
> > > > 
> > > 
> > > No, even on i386 gcov_type is 64 bit.  I have just double checked.
> > > I have committed the patch as it was per Honza's approval.
> > 
> > But what's the point moving it first, even before the next pointer?
> > The next pointer is accessed during traversing, possibly quite more
> > frequent than count.
> > 
> 
> Well, after I saw how misaligned everything was (and so how little we,
> including myself, sometimes care about this when we add stuff), I
> thought that reordering the fields according to their size was the
> safest way of preventing someone from inadvertently reintroducing the
> padding.  Even more so because nobody seems to really remember how big
> gcov_type actually is (I also had to ask Honza, probably not for the
> first time).
> 
> Having said that, putting it after next_callee with a comment about
> this is probably also rather safe (as opposed to anything lower than
> that).  I can do that if you want.

Well, I don't care too much - it's just another thing I do when
re-arranging fields, put pointer chains first.

Richard.

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Martin Jambor]

Hi,

On Tue, Dec 01, 2009 at 12:33:38PM +0100, Richard Guenther wrote:
> On Tue, 1 Dec 2009, Martin Jambor wrote:
> > On Tue, Dec 01, 2009 at 12:04:58PM +0100, Richard Guenther wrote:
> > > On Tue, 1 Dec 2009, Martin Jambor wrote:
> > > > On Tue, Dec 01, 2009 at 10:46:33AM +0100, Richard Guenther wrote:
> > > > > 
> > > > > you now have introduced 32bit padding here ...
> > > > > 
> > > > 
> > > > No, even on i386 gcov_type is 64 bit.  I have just double checked.
> > > > I have committed the patch as it was per Honza's approval.
> > > 
> > > But what's the point moving it first, even before the next pointer?
> > > The next pointer is accessed during traversing, possibly quite more
> > > frequent than count.
> > > 
> > 
> > Well, after I saw how misaligned everything was (and so how little we,
> > including myself, sometimes care about this when we add stuff), I
> > thought that reordering the fields according to their size was the
> > safest way of preventing someone from inadvertently reintroducing the
> > padding.  Even more so because nobody seems to really remember how big
> > gcov_type actually is (I also had to ask Honza, probably not for the
> > first time).
> > 
> > Having said that, putting it after next_callee with a comment about
> > this is probably also rather safe (as opposed to anything lower than
> > that).  I can do that if you want.
> 
> Well, I don't care too much - it's just another thing I do when
> re-arranging fields, put pointer chains first.
> 

No problem, the following has bootstrapped and tested on an
x86_64-linux overnight.  We probably wouldn't even be causeing much
fuss by committing it.

Martin


2009-12-02  Martin Jambor  <mjambor@suse.cz>

	* cgraph.h (struct cgraph_edge): Move count after next_callee.

Index: icln/gcc/cgraph.h
===================================================================
--- icln.orig/gcc/cgraph.h
+++ icln/gcc/cgraph.h
@@ -310,14 +310,15 @@ typedef enum {
 } cgraph_inline_failed_t;
 
 struct GTY((chain_next ("%h.next_caller"), chain_prev ("%h.prev_caller"))) cgraph_edge {
-  /* Expected number of executions: calculated in profile.c.  */
-  gcov_type count;
   struct cgraph_node *caller;
   struct cgraph_node *callee;
   struct cgraph_edge *prev_caller;
   struct cgraph_edge *next_caller;
   struct cgraph_edge *prev_callee;
   struct cgraph_edge *next_callee;
+  /* Expected number of executions: calculated in profile.c.  This field should
+     be aligned to 64 bits on all targets.  */
+  gcov_type count;
   gimple call_stmt;
   PTR GTY ((skip (""))) aux;
   /* When equal to CIF_OK, inline this call.  Otherwise, points to the

Re: [PATCH] Reorder fields in struct cgraph_edge et al

[Richard Henderson]

On 12/02/2009 04:00 AM, Martin Jambor wrote:
> +  /* Expected number of executions: calculated in profile.c.  This field should
> +     be aligned to 64 bits on all targets.  */
> +  gcov_type count;

Comment is wrong.  The field is 64-bits *wide* on all targets.
But on 32-bit targets, 64-bit types are often 32-bit aligned.


r~