[Bug tree-optimization/40744] New: SRA scalarizes dead objects, single-use objects

[Bug tree-optimization/40744] New: SRA scalarizes dead objects, single-use objects

[rguenth at gcc dot gnu dot org]

struct X { int i; int j; };
void foo(void)
{
  struct X x;
  x.i = 1;
  x.j = 2;
}

early SRA produces

foo ()
{
  int x$j;
  int x$i;
  struct X x;

<bb 2>:
  x$i_3 = 1;
  x$j_2 = 2;
  return;


which is unnecessary work as DCE will end up removing the stores anyway.
We should avoid doing useless work here (and thus not skew statistics
compared to when somebody inserts DCE before ESRA).  At least if it is
easy to do.

Likewise for

struct X { int i; int j; };
int foo(struct X x)
{
  return x.i;
}

early SRA produces an extra register copy with no benefit.

foo (struct X x)
{
  int x$i;
  int D.1606;

<bb 2>:
  x$i_3 = x.i;
  D.1606_1 = x$i_3;
  return D.1606_1;

}

both cases, only loads from a structure or only stores to a structure
probably should be simply skipped.


-- 
           Summary: SRA scalarizes dead objects, single-use objects
           Product: gcc
           Version: 4.5.0
            Status: UNCONFIRMED
          Keywords: compile-time-hog
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: rguenth at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[jamborm at gcc dot gnu dot org]

------- Comment #1 from jamborm at gcc dot gnu dot org  2009-07-14 16:32 -------
OK, I have now added this to my todo list.  The simple tweaks would be
simple.  On  the other hand, if  DCE is clever, it  still might figure
out a structure is dead at  some code paths while I don't even attempt
to and still remove some statements.

> only loads from a structure or only stores to a structure
> probably should be simply skipped.

I've been thinking about not scalarizing accesses that occur only once
too.  Thinking about it more,  I wonder whether requiring at least one
load and  a store or  at least  two loads is  a good idea.   (Load and
store for the case where we  store something to an aggregate first and
then load it and  two loads for the case when we  can finally get away
with one.)

I'll try to come up with something this simple quickly and hopefully
we can benchmark it over the weekend or so...


-- 

jamborm at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|mjambor at suse dot cz      |jamborm at gcc dot gnu dot
                   |                            |org
         AssignedTo|unassigned at gcc dot gnu   |jamborm at gcc dot gnu dot
                   |dot org                     |org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-07-14 16:32:37
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[rguenther at suse dot de]

------- Comment #2 from rguenther at suse dot de  2009-07-14 21:28 -------
Subject: Re:  SRA scalarizes dead objects,
 single-use objects

On Tue, 14 Jul 2009, jamborm at gcc dot gnu dot org wrote:

> ------- Comment #1 from jamborm at gcc dot gnu dot org  2009-07-14 16:32 -------
> OK, I have now added this to my todo list.  The simple tweaks would be
> simple.  On  the other hand, if  DCE is clever, it  still might figure
> out a structure is dead at  some code paths while I don't even attempt
> to and still remove some statements.
> 
> > only loads from a structure or only stores to a structure
> > probably should be simply skipped.
> 
> I've been thinking about not scalarizing accesses that occur only once
> too.  Thinking about it more,  I wonder whether requiring at least one
> load and  a store or  at least  two loads is  a good idea.   (Load and
> store for the case where we  store something to an aggregate first and
> then load it and  two loads for the case when we  can finally get away
> with one.)

Note that it is important to keep the structure copyprop case

 tmp = b;
 return tmp;

which happens quite often in C++ code.

Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[jamborm at gcc dot gnu dot org]

------- Comment #3 from jamborm at gcc dot gnu dot org  2009-07-30 14:18 -------
Richi, not scalarizing things like the second foo() in the original
bug description will inevitably lead to warning failures in
g++.dg/warn/unit-1.C and gcc.dg/uninit-I.c.  Is that OK?  Should I
remove or XFAIl them?

(Structure copy-prop is being checked on gcc.dg/tree-ssa/sra-6, so
that is safe.)

Hopefully I will attach a patch here soon.

Martin


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[rguenther at suse dot de]

------- Comment #4 from rguenther at suse dot de  2009-07-30 14:29 -------
Subject: Re:  SRA scalarizes dead objects,
 single-use objects

On Thu, 30 Jul 2009, jamborm at gcc dot gnu dot org wrote:

> ------- Comment #3 from jamborm at gcc dot gnu dot org  2009-07-30 14:18 -------
> Richi, not scalarizing things like the second foo() in the original
> bug description will inevitably lead to warning failures in
> g++.dg/warn/unit-1.C and gcc.dg/uninit-I.c.  Is that OK?  Should I
> remove or XFAIl them?

XFAIL them.

> (Structure copy-prop is being checked on gcc.dg/tree-ssa/sra-6, so
> that is safe.)

Good.

> Hopefully I will attach a patch here soon.

Thx.
Richard.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[jamborm at gcc dot gnu dot org]

------- Comment #5 from jamborm at gcc dot gnu dot org  2009-07-30 17:07 -------
Created an attachment (id=18273)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18273&action=view)
Proposed patch

The attached patch  does turn SRA down a  bit.  Specifically, 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.)

I'm bootstrapping and testing this at the moment but only to find out
that the testcase changes are OK, the last bootstrap with exactly
these changes to tree-sra.c finished fine.  I want to have benchmarks
run on this, however, to make sure I do not do any harm.  I expect to
be benchmarking ipa-cp cloning the next couple of days and do this
right afterwards, I guess early next week.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[jamborm at gcc dot gnu dot org]

------- Comment #6 from jamborm at gcc dot gnu dot org  2009-07-30 17:10 -------
Created an attachment (id=18274)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18274&action=view)
Proposed patch

Well, apparently I forgot to run quilt refresh, this is the current
patch with the testcase changes.


-- 

jamborm at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18273|0                           |1
        is obsolete|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[rguenther at suse dot de]

------- Comment #7 from rguenther at suse dot de  2009-07-31 09:12 -------
Subject: Re:  SRA scalarizes dead objects,
 single-use objects

On Thu, 30 Jul 2009, jamborm at gcc dot gnu dot org wrote:

> ------- Comment #6 from jamborm at gcc dot gnu dot org  2009-07-30 17:10 -------
> Created an attachment (id=18274)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18274&action=view)
>  --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=18274&action=view)
> Proposed patch
> 
> Well, apparently I forgot to run quilt refresh, this is the current
> patch with the testcase changes.

Looks good to me.

Thanks.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[jamborm at gcc dot gnu dot org]

------- Comment #8 from jamborm at gcc dot gnu dot org  2009-09-02 18:03 -------
Hi,

I have committed the patch as revision 151345 after another
bootstrap/testing.  Unfortunately I forgot to annotate them with this
PR number in the change log and so the commits did not appear here
automatically.  I will try to remember the next time.

Thus, this should be fixed.


-- 

jamborm at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744

[Bug tree-optimization/40744] SRA scalarizes dead objects, single-use objects

[ubizjak at gmail dot com]

------- Comment #9 from ubizjak at gmail dot com  2009-09-03 08:47 -------
You can just copy relevant entry from gcc-cvs ml:
Author: jamborm
Date: Wed Sep  2 17:52:18 2009
New Revision: 151345

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=151345
Log:
2009-09-02  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 of 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.


Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/sra-8.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/warn/unit-1.C
    trunk/gcc/testsuite/gcc.dg/memcpy-1.c
    trunk/gcc/testsuite/gcc.dg/uninit-I.c
    trunk/gcc/tree-sra.c


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40744