This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Don't completely scalarize a record if it contains bit-field (PR tree-optimization/45144)


Hi,

On Sat, Jul 31, 2010 at 12:09:42AM +0800, Jie Zhang wrote:
> PR tree-optimization/45144 shows an issue that SRA causes. I used
> arm-none-eabi target as an example in PR tree-optimization/45144.
> But the same issue can also been seen on x86_64-linux-gnu target
> using the same test case in the PR.
> 
> SRA completely scalarizes a small record. But when the record is
> used later as a whole, GCC has to make the record out of the scalar
> parts. When the record contains bit-fields, GCC generates ugly code
> to assemble the scalar parts into a record.
> 
> Until the aggregates copy propagation is implemented, I think it
> would better to disable full scalarization for such records. The
> patch is attached. It's bootstrapped on x86_64-linux-gnu and
> regression tested.
> 
> Is it OK for now? We can remove it after aggregates copy propagation
> is implemented.
> 
> Will it be better to add bit-field check in
> type_consists_of_records_p instead of using a new function
> "type_contains_bit_field_p"?
> 

When I was implementing the total scalarization bit of SRA I thought
of disabling it for structures with bit-fields too.  I did not really
examine the effects in any way but I never expected this to result in
nice code at places where we use SRA to do poor-man's copy
propagation.  However, eventually I decided to keep the total
scalarization for these structures because doing so can save stack
space and it would be shame if adding one such field to a structure
would make us use the space again (in fact, total scalarization was
introduced as a fix to unnecessary stack-frame setup bugs like PR
42585).  But given your results with kernel and gcc, I don't object to
disabling it... people will scream if something slows down for them.

On the other hand, if we decide to go this way, we need to do the
check at a different place, going over the whole type whenever looking
at an assignment is not necessary and is wasteful.  The check would be
most appropriate as a part of type_consists_of_records_p where it
would be performed only once for each variable in question.

Thanks,

Martin

> 
> Regards,
> -- 
> Jie Zhang
> CodeSourcery

> 
> 	PR tree-optimization/45144
> 	* tree-sra.c (type_contains_bit_field_p): New.
> 	(build_accesses_from_assign): Don't completely scalarize
> 	a record if it contains bit-field.
> 
> 	testsuite/
> 	PR tree-optimization/45144
> 	* gcc.dg/tree-ssa/pr45144.c: New test.
> 
> Index: testsuite/gcc.dg/tree-ssa/pr45144.c
> ===================================================================
> --- testsuite/gcc.dg/tree-ssa/pr45144.c	(revision 0)
> +++ testsuite/gcc.dg/tree-ssa/pr45144.c	(revision 0)
> @@ -0,0 +1,46 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +void baz (unsigned);
> +
> +extern unsigned buf[];
> +
> +struct A
> +{
> +  unsigned a1:10;
> +  unsigned a2:3;
> +  unsigned:19;
> +};
> +
> +union TMP
> +{
> +  struct A a;
> +  unsigned int b;
> +};
> +
> +static unsigned
> +foo (struct A *p)
> +{
> +  union TMP t;
> +  struct A x;
> +  
> +  x = *p;
> +  t.a = x;
> +  return t.b;
> +}
> +
> +void
> +bar (unsigned orig, unsigned *new)
> +{
> +  struct A a;
> +  union TMP s;
> +
> +  s.b = orig;
> +  a = s.a;
> +  if (a.a1)
> +    baz (a.a2);
> +  *new = foo (&a);
> +}
> +
> +/* { dg-final { scan-tree-dump "x = a;" "optimized"} } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c	(revision 162704)
> +++ tree-sra.c	(working copy)
> @@ -998,6 +998,30 @@ disqualify_ops_if_throwing_stmt (gimple 
>    return false;
>  }
>  
> +static bool
> +type_contains_bit_field_p (const_tree type)
> +{
> +  tree fld;
> +
> +  if (TREE_CODE (type) != RECORD_TYPE)
> +    return false;
> +
> +  for (fld = TYPE_FIELDS (type); fld; fld = DECL_CHAIN (fld))
> +    if (TREE_CODE (fld) == FIELD_DECL)
> +      {
> +	tree ft = TREE_TYPE (fld);
> +
> +	if (DECL_BIT_FIELD (fld))
> +	  return true;
> +
> +	if (TREE_CODE (ft) == RECORD_TYPE
> +	    && type_contains_bit_field_p (ft))
> +	  return true;
> +      }
> +
> +  return false;
> +}
> +
>  /* Scan expressions occuring in STMT, create access structures for all accesses
>     to candidates for scalarization and remove those candidates which occur in
>     statements or expressions that prevent them from being split apart.  Return
> @@ -1025,7 +1049,8 @@ build_accesses_from_assign (gimple stmt)
>      {
>        racc->grp_assignment_read = 1;
>        if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt)
> -	  && !is_gimple_reg_type (racc->type))
> +	  && !is_gimple_reg_type (racc->type)
> +	  && !type_contains_bit_field_p (racc->type))
>  	bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base));
>      }
>  


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]