This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, PR 40744] Turn down SRA a bit
- From: Martin Jambor <mjambor at suse dot cz>
- To: gcc-patches at gcc dot gnu dot org
- Date: Wed, 2 Sep 2009 19:54:45 +0200
- Subject: Re: [PATCH, PR 40744] Turn down SRA a bit
- References: <20090806182345.GE24310@virgil.suse.cz> <alpine.LNX.2.00.0908071023380.16347@zhemvz.fhfr.qr>
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