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.
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