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