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

[Jie Zhang <jie at codesourcery dot com>]

On 08/02/2010 09:01 PM, Martin Jambor wrote:
> 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 for your comment! How about this version? I moved the check into 
type_consists_of_records_p. Bootstrapped and regression tested on 
x86_64. Also checked by building linux kernel and made sure there were 
no regressions.


Regards,
--
Jie Zhang
CodeSourcery

	PR tree-optimization/45144
	* tree-sra.c (type_consists_of_records_p): Return false
	if the record 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 162725)
+++ tree-sra.c	(working copy)
@@ -811,7 +811,7 @@ create_access (tree expr, gimple stmt, b
 /* Return true iff TYPE is a RECORD_TYPE with fields that are either of gimple
    register types or (recursively) records with only these two kinds of fields.
    It also returns false if any of these records has a zero-size field as its
-   last field.  */
+   last field or has a bit-field.  */
 
 static bool
 type_consists_of_records_p (tree type)
@@ -827,6 +827,9 @@ type_consists_of_records_p (tree type)
       {
 	tree ft = TREE_TYPE (fld);
 
+	if (DECL_BIT_FIELD (fld))
+	  return false;
+
 	if (!is_gimple_reg_type (ft)
 	    && !type_consists_of_records_p (ft))
 	  return false;