| Summary: | SRA scalarizes dead objects, single-use objects | ||
|---|---|---|---|
| Product: | gcc | Reporter: | Richard Biener <rguenth> |
| Component: | tree-optimization | Assignee: | Martin Jambor <jamborm> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | gcc-bugs, jamborm |
| Priority: | P3 | Keywords: | compile-time-hog |
| Version: | 4.5.0 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | 2009-07-14 16:32:37 | |
| Attachments: |
Proposed patch
Proposed patch |
||
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...
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.
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 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. Created attachment 18273 [details]
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.
Created attachment 18274 [details]
Proposed patch
Well, apparently I forgot to run quilt refresh, this is the current
patch with the testcase changes.
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)
> Proposed patch
>
> Well, apparently I forgot to run quilt refresh, this is the current
> patch with the testcase changes.
Looks good to me.
Thanks.
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. 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 |
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.