Re: [PATCH, PR 40744] Turn down SRA a bit

[Richard Guenther <rguenther at suse dot de>]

On Thu, 6 Aug 2009, Martin Jambor wrote:

> Hi,
> 
> PR 40744  is a request  not to create  scalar replacements when  it is
> clearly not  beneficial, for example  with dead objects or  single use
> objects.  I  have come up with a  patch that is in  bugzilla since the
> last week.  I was afraid it might cause some run time regressions so I
> put it on  a periodic tester, which initially  showed problems but the
> last run  suggests it was all just  noise.  There fore I  post it here
> now (I am also re-bootstrapping and testing it on x86_64-linux).
> 
> This patch changes SRA in such a way that in order
> to create a replacement, the corresponding access (group) must either:
> 
> - be read individually multiple times or
> 
> - be read individually and also written to (either individually or
>   through its parent) or
> 
> - somehow accessed individually and be on the RHS of structure
>   copy-prop link or
> 
> - be read individually and be on the LHS of structure copy-prop link.
> 
> (The bottom line is to avoid scalarizing accesses with only stores or
> just one read - and no stores.  Another thing to be noted is that with
> this patch we also insist the access must be at least once read
> individually, not as a part of its parent to be scalarized.)
> 
> OK for trunk if the re-bootstrap and re-testing goes fine?

Yes,

thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 2009-07-30  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (struct access): New field grp_hint.
> 	(dump_access): Dump grp_hint.
> 	(sort_and_splice_var_accesses): Set grp_hint if a group is read
> 	multiple times.
> 	(analyze_access_subtree): Only scalarize accesses with grp_hint set or
> 	those which have been specifically read and somehow written to.
> 	(propagate_subacesses_accross_link): Set grp_hint of right child and
> 	also possibly in the left child.
> 
> 	* testsuite/gcc.dg/tree-ssa/sra-8.c: New testcase.
> 	* testsuite/gcc.dg/memcpy-1.c: Add . to match pattern.
> 	* testsuite/gcc.dg/uninit-I.c: XFAIL warning test.
> 	* testsuite/g++.dg/warn/unit-1.C: XFAIL warning test.
> 
> 
> 	
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -164,6 +164,10 @@ struct access
>    /* Does this group contain a read access?  This flag is propagated down the
>       access tree.  */
>    unsigned grp_read : 1;
> +  /* Other passes of the analysis use this bit to make function
> +     analyze_access_subtree create scalar replacements for this group if
> +     possible.  */
> +  unsigned grp_hint : 1;
>    /* Is the subtree rooted in this access fully covered by scalar
>       replacements?  */
>    unsigned grp_covered : 1;
> @@ -260,12 +264,14 @@ dump_access (FILE *f, struct access *acc
>    fprintf (f, ", type = ");
>    print_generic_expr (f, access->type, 0);
>    if (grp)
> -    fprintf (f, ", grp_write = %d, grp_read = %d, grp_covered = %d, "
> -	     "grp_unscalarizable_region = %d, grp_unscalarized_data = %d, "
> -	     "grp_partial_lhs = %d, grp_to_be_replaced = %d\n",
> -	     access->grp_write, access->grp_read, access->grp_covered,
> -	     access->grp_unscalarizable_region, access->grp_unscalarized_data,
> -	     access->grp_partial_lhs, access->grp_to_be_replaced);
> +    fprintf (f, ", grp_write = %d, grp_read = %d, grp_hint = %d, "
> +	     "grp_covered = %d, grp_unscalarizable_region = %d, "
> +	     "grp_unscalarized_data = %d, grp_partial_lhs = %d, "
> +	     "grp_to_be_replaced = %d\n",
> +	     access->grp_write, access->grp_read, access->grp_hint,
> +	     access->grp_covered, access->grp_unscalarizable_region,
> +	     access->grp_unscalarized_data, access->grp_partial_lhs,
> +	     access->grp_to_be_replaced);
>    else
>      fprintf (f, ", write = %d, grp_partial_lhs = %d\n", access->write,
>  	     access->grp_partial_lhs);
> @@ -1202,8 +1208,9 @@ sort_and_splice_var_accesses (tree var)
>    while (i < access_count)
>      {
>        struct access *access = VEC_index (access_p, access_vec, i);
> -      bool modification = access->write;
> +      bool grp_write = access->write;
>        bool grp_read = !access->write;
> +      bool multiple_reads = false;
>        bool grp_partial_lhs = access->grp_partial_lhs;
>        bool first_scalar = is_gimple_reg_type (access->type);
>        bool unscalarizable_region = access->grp_unscalarizable_region;
> @@ -1226,8 +1233,15 @@ sort_and_splice_var_accesses (tree var)
>  	  struct access *ac2 = VEC_index (access_p, access_vec, j);
>  	  if (ac2->offset != access->offset || ac2->size != access->size)
>  	    break;
> -	  modification |= ac2->write;
> -	  grp_read |= !ac2->write;
> +	  if (ac2->write)
> +	    grp_write = true;
> +	  else
> +	    {
> +	      if (grp_read)
> +		multiple_reads = true;
> +	      else
> +		grp_read = true;
> +	    }
>  	  grp_partial_lhs |= ac2->grp_partial_lhs;
>  	  unscalarizable_region |= ac2->grp_unscalarizable_region;
>  	  relink_to_new_repr (access, ac2);
> @@ -1243,8 +1257,9 @@ sort_and_splice_var_accesses (tree var)
>        i = j;
>  
>        access->group_representative = access;
> -      access->grp_write = modification;
> +      access->grp_write = grp_write;
>        access->grp_read = grp_read;
> +      access->grp_hint = multiple_reads;
>        access->grp_partial_lhs = grp_partial_lhs;
>        access->grp_unscalarizable_region = unscalarizable_region;
>        if (access->first_link)
> @@ -1376,6 +1391,7 @@ analyze_access_subtree (struct access *r
>    HOST_WIDE_INT covered_to = root->offset;
>    bool scalar = is_gimple_reg_type (root->type);
>    bool hole = false, sth_created = false;
> +  bool direct_read = root->grp_read;
>  
>    if (mark_read)
>      root->grp_read = true;
> @@ -1404,7 +1420,9 @@ analyze_access_subtree (struct access *r
>        hole |= !child->grp_covered;
>      }
>  
> -  if (allow_replacements && scalar && !root->first_child)
> +  if (allow_replacements && scalar && !root->first_child
> +      && (root->grp_hint
> +	  || (direct_read && root->grp_write)))
>      {
>        if (dump_file && (dump_flags & TDF_DETAILS))
>  	{
> @@ -1542,7 +1560,6 @@ propagate_subacesses_accross_link (struc
>  {
>    struct access *rchild;
>    HOST_WIDE_INT norm_delta = lacc->offset - racc->offset;
> -
>    bool ret = false;
>  
>    if (is_gimple_reg_type (lacc->type)
> @@ -1569,8 +1586,13 @@ propagate_subacesses_accross_link (struc
>        if (child_would_conflict_in_lacc (lacc, norm_offset, rchild->size,
>  					&new_acc))
>  	{
> -	  if (new_acc && rchild->first_child)
> -	    ret |= propagate_subacesses_accross_link (new_acc, rchild);
> +	  if (new_acc)
> +	    {
> +	      rchild->grp_hint = 1;
> +	      new_acc->grp_hint |= new_acc->grp_read;
> +	      if (rchild->first_child)
> +		ret |= propagate_subacesses_accross_link (new_acc, rchild);
> +	    }
>  	  continue;
>  	}
>  
> @@ -1581,6 +1603,7 @@ propagate_subacesses_accross_link (struc
>  				 rchild->type, false))
>  	continue;
>  
> +      rchild->grp_hint = 1;
>        new_acc = create_artificial_child_access (lacc, rchild, norm_offset);
>        if (racc->first_child)
>  	propagate_subacesses_accross_link (new_acc, rchild);
> Index: mine/gcc/testsuite/gcc.dg/tree-ssa/sra-8.c
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gcc.dg/tree-ssa/sra-8.c
> @@ -0,0 +1,35 @@
> +/* A testcase for PR 40744.  We do not want to create replacements for object
> +   that are dead or have only a single use, whenever it can be avoided
> +   simply. */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -fdump-tree-esra-details" } */
> +
> +struct X { int i; int j; };
> +
> +void foo(void)
> +{
> +  struct X x;
> +  x.i = 1;
> +  x.j = 2;
> +}
> +
> +
> +int bar(struct X x)
> +{
> +  return x.i;
> +}
> +
> +
> +extern void do_something (struct X);
> +
> +void bar2(int i, int j)
> +{
> +  struct X x;
> +
> +  x.i = i;
> +  x.j = j;
> +  do_something (x);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "Created a replacement" 0 "esra"} } */
> +/* { dg-final { cleanup-tree-dump "esra" } } */
> Index: mine/gcc/testsuite/g++.dg/warn/unit-1.C
> ===================================================================
> --- mine.orig/gcc/testsuite/g++.dg/warn/unit-1.C
> +++ mine/gcc/testsuite/g++.dg/warn/unit-1.C
> @@ -4,7 +4,7 @@
>  struct a { int mode; };
>  int sys_msgctl (void)
>  {
> -  struct a setbuf;  /* { dg-warning "'setbuf\.a::mode' is used" } */
> +  struct a setbuf;  /* { dg-warning "'setbuf\.a::mode' is used" "" { xfail *-*-* } } */
>    return setbuf.mode;
>  }
>  
> Index: mine/gcc/testsuite/gcc.dg/memcpy-1.c
> ===================================================================
> --- mine.orig/gcc/testsuite/gcc.dg/memcpy-1.c
> +++ mine/gcc/testsuite/gcc.dg/memcpy-1.c
> @@ -1,7 +1,7 @@
>  /* { dg-do compile } */
>  /* { dg-options "-O2 -fdump-tree-optimized" } */
>  /* PR36598 AVR fail maybe due to cost metrics */
> -/* { dg-final { scan-tree-dump-times "nasty_local" 0 "optimized" { xfail { "avr-*-*" } } } } */
> +/* { dg-final { scan-tree-dump-times "nasty_local\\." 0 "optimized" { xfail { "avr-*-*" } } } } */
>  /* { dg-final { cleanup-tree-dump "optimized" } } */
>  struct a {int a,b,c;} a;
>  int test(struct a a)
> Index: mine/gcc/testsuite/gcc.dg/uninit-I.c
> ===================================================================
> --- mine.orig/gcc/testsuite/gcc.dg/uninit-I.c
> +++ mine/gcc/testsuite/gcc.dg/uninit-I.c
> @@ -3,6 +3,6 @@
>  
>  int sys_msgctl (void)
>  {
> -  struct { int mode; } setbuf;  /* { dg-warning "'setbuf\.mode' is used" } */
> +  struct { int mode; } setbuf;  /* { dg-warning "'setbuf\.mode' is used" "" { xfail *-*-* } } */
>    return setbuf.mode;
>  }
> 
> 

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