From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3479 invoked by alias); 25 Aug 2015 22:42:17 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 3454 invoked by uid 89); 25 Aug 2015 22:42:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Tue, 25 Aug 2015 22:42:13 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1821DAAF3; Tue, 25 Aug 2015 22:42:09 +0000 (UTC) Date: Tue, 25 Aug 2015 22:51:00 -0000 From: Martin Jambor To: Alan Lawrence Cc: gcc-patches@gcc.gnu.org, rguenther@suse.de Subject: Re: [PATCH 3/5] Build ARRAY_REFs when the base is of ARRAY_TYPE. Message-ID: <20150825224207.GC12831@virgil.suse.cz> Mail-Followup-To: Alan Lawrence , gcc-patches@gcc.gnu.org, rguenther@suse.de References: <1440500777-25966-1-git-send-email-alan.lawrence@arm.com> <1440500777-25966-4-git-send-email-alan.lawrence@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1440500777-25966-4-git-send-email-alan.lawrence@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg01565.txt.bz2 Hi, On Tue, Aug 25, 2015 at 12:06:15PM +0100, 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. Uff. Well, while I am obviously not excited about such workaround landing in SRA, if maintainers agree that it is reasonable, I suppose I'll manage to live with it. I also have more specific comments: > > 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. > > 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) As Jeff already pointed out, this function needs a comment. > +{ > + gcc_assert (TREE_CODE (type) == ARRAY_TYPE); > + tree t_size = TYPE_SIZE (TREE_TYPE (type)); > + if (!t_size || !tree_fits_uhwi_p (t_size)) > + return false; > + unsigned HOST_WIDE_INT sz = tree_to_uhwi (t_size); > + if (!sz) > + return false; > + *sz_out = sz; > + return true; > +} > + > static void scalarize_elem (tree, HOST_WIDE_INT, HOST_WIDE_INT, tree, tree); > > /* Create total_scalarization accesses for all scalar fields of a member > @@ -985,10 +999,9 @@ completely_scalarize (tree base, tree decl_type, HOST_WIDE_INT offset, tree ref) > case ARRAY_TYPE: > { > tree elemtype = TREE_TYPE (decl_type); > - tree elem_size = TYPE_SIZE (elemtype); > - gcc_assert (elem_size && tree_fits_uhwi_p (elem_size)); > - int el_size = tree_to_uhwi (elem_size); > - gcc_assert (el_size); > + unsigned HOST_WIDE_INT el_size; > + if (!get_elem_size (decl_type, &el_size)) > + gcc_assert (false); This is usually written as gcc_unreachable () > > tree minidx = TYPE_MIN_VALUE (TYPE_DOMAIN (decl_type)); > tree maxidx = TYPE_MAX_VALUE (TYPE_DOMAIN (decl_type)); > @@ -1563,7 +1576,7 @@ build_ref_for_offset (location_t loc, tree base, HOST_WIDE_INT offset, > tree off; > tree mem_ref; > HOST_WIDE_INT base_offset; > - unsigned HOST_WIDE_INT misalign; > + unsigned HOST_WIDE_INT misalign, el_sz; > unsigned int align; > > gcc_checking_assert (offset % BITS_PER_UNIT == 0); > @@ -1572,47 +1585,62 @@ build_ref_for_offset (location_t loc, tree base, HOST_WIDE_INT offset, > > /* get_addr_base_and_unit_offset returns NULL for references with a variable > offset such as array[var_index]. */ > - if (!base) > - { > - gassign *stmt; > - tree tmp, addr; > - > - gcc_checking_assert (gsi); > - tmp = make_ssa_name (build_pointer_type (TREE_TYPE (prev_base))); > - addr = build_fold_addr_expr (unshare_expr (prev_base)); > - STRIP_USELESS_TYPE_CONVERSION (addr); > - stmt = gimple_build_assign (tmp, addr); > - gimple_set_location (stmt, loc); > - if (insert_after) > - gsi_insert_after (gsi, stmt, GSI_NEW_STMT); > - else > - gsi_insert_before (gsi, stmt, GSI_SAME_STMT); > - > - off = build_int_cst (reference_alias_ptr_type (prev_base), > - offset / BITS_PER_UNIT); > - base = tmp; > - } > - else if (TREE_CODE (base) == MEM_REF) > - { > - off = build_int_cst (TREE_TYPE (TREE_OPERAND (base, 1)), > - base_offset + offset / BITS_PER_UNIT); > - off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), off); > - base = unshare_expr (TREE_OPERAND (base, 0)); > + if (base > + && TREE_CODE (TREE_TYPE (base)) == ARRAY_TYPE > + && misalign == 0 > + && get_elem_size (TREE_TYPE (base), &el_sz) > + && ((offset % el_sz) == 0) > + && useless_type_conversion_p (exp_type, TREE_TYPE (TREE_TYPE (base))) > + && (align >= TYPE_ALIGN (exp_type))) If this goes through, please add a comment explaining why we are doing this, especially if you do not have a test-case. > + { > + tree idx = build_int_cst (TYPE_DOMAIN (TREE_TYPE (base)), offset / el_sz); > + base = unshare_expr (base); > + mem_ref = build4 (ARRAY_REF, exp_type, base, idx, NULL_TREE, NULL_TREE); Since you are changing the function not to always produce a MEM_REF, you need to adjust its comment again ;-) Thanks, Martin