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: [PATCH, PR 40744] Turn down SRA a bit


On Fri, Aug 07, 2009 at 10:23:47AM +0200, Richard Guenther wrote:
> 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,
> 

Because a lot of time has passed since the approval I have
bootstrapped and regression tested this again today (rev.  151322) on
an x86-64 and there were still no problems and so I comitted it as
revision 151345.

Thanks,

Martin



> 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


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