On Wed, Aug 26, 2015 at 3:29 PM, Richard Biener <rguent...@suse.de> wrote: > On Wed, 26 Aug 2015, Bin.Cheng wrote: > >> On Wed, Aug 26, 2015 at 3:50 AM, Jeff Law <l...@redhat.com> wrote: >> > On 08/25/2015 05:06 AM, Alan Lawrence wrote: >> >> >> >> When SRA completely scalarizes an array, this patch changes the >> >> generated accesses from e.g. >> >> >> >> MEM[(int[8] *)&a + 4B] = 1; >> >> >> >> to >> >> >> >> a[1] = 1; >> >> >> >> This overcomes a limitation in dom2, that accesses to equivalent >> >> chunks of e.g. MEM[(int[8] *)&a] are not hashable_expr_equal_p with >> >> accesses to e.g. MEM[(int[8] *)&a]. This is necessary for constant >> >> propagation in the ssa-dom-cse-2.c testcase (after the next patch >> >> that makes SRA handle constant-pool loads). >> >> >> >> I tried to work around this by making dom2's hashable_expr_equal_p >> >> less conservative, but found that on platforms without AArch64's >> >> vectorized reductions (specifically Alpha, hppa, PowerPC, and SPARC, >> >> mentioned in ssa-dom-cse-2.c), I also needed to make MEM[(int[8] >> >> *)&a] equivalent to a[0], etc.; a complete overhaul of >> >> hashable_expr_equal_p seems like a larger task than this patch >> >> series. >> >> >> >> I can't see how to write a testcase for this in C though as direct >> >> assignment to an array is not possible; such assignments occur only >> >> with constant pool data, which is dealt with in the next patch. >> > >> > It's a general issue that if there's > 1 common way to represent an >> > expression, then DOM will often miss discovery of the CSE opportunity >> > because of the way it hashes expressions. >> > >> > Ideally we'd be moving to a canonical form, but I also realize that in >> > the case of memory references like this, that may not be feasible. >> IIRC, there were talks about lowering all memory reference on GIMPLE? >> Which is the reverse approach. Since SRA is in quite early >> compilation stage, don't know if lowered memory reference has impact >> on other optimizers. > > Yeah, I'd only do the lowering after loop opts. Which also may make > the DOM issue moot as the array refs would be lowered as well and thus > DOM would see a consistent set of references again. The lowering should > also simplify SLSR and expose address computation redundancies to DOM. > > I'd place such lowering before the late reassoc (any takers? I suppose > you can pick up one of the bitfield lowering passes posted in the > previous years as this should also handle bitfield accesses correctly). I ran into several issues related to lowered memory references (some of them are about slsr), and want to have a look at this. But only after finishing major issues in IVO...
As for slsr, I think the problem is more about we need to prove equality of expressions by diving into definition chain of ssa_var, just like tree_to_affine_expand. I think this has already been discussed too. Anyway, lowering memory reference provides a canonical form and should benefit other optimizers. Thanks, bin > > Thanks, > Richard. > >> Thanks, >> bin >> > >> > It does make me wonder how many CSEs we're really missing due to the two >> > ways to represent array accesses. >> > >> > >> >> Bootstrap + check-gcc on x86-none-linux-gnu, >> >> arm-none-linux-gnueabihf, aarch64-none-linux-gnu. >> >> >> >> gcc/ChangeLog: >> >> >> >> * tree-sra.c (completely_scalarize): Move some code into: >> >> (get_elem_size): New. (build_ref_for_offset): Build ARRAY_REF if base >> >> is aligned array. --- gcc/tree-sra.c | 110 >> >> ++++++++++++++++++++++++++++++++++++--------------------- 1 file >> >> changed, 69 insertions(+), 41 deletions(-) >> >> >> >> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 08fa8dc..af35fcc >> >> 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -957,6 +957,20 @@ >> >> scalarizable_type_p (tree type) } } >> >> >> >> +static bool +get_elem_size (const_tree type, unsigned HOST_WIDE_INT >> >> *sz_out) >> > >> > Function comment needed. >> > >> > I may have missed it in the earlier patches, but can you please make >> > sure any new functions you created have comments in those as well. Such >> > patches are pre-approved. >> > >> > With the added function comment, this patch is fine. >> > >> > jeff >> > >> > >> >> > > -- > Richard Biener <rguent...@suse.de> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB > 21284 (AG Nuernberg)
