From patchwork Fri May 20 09:41:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 68231 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp3672648qge; Fri, 20 May 2016 02:42:23 -0700 (PDT) X-Received: by 10.98.74.142 with SMTP id c14mr3238559pfj.63.1463737342942; Fri, 20 May 2016 02:42:22 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id 6si26851818pfh.24.2016.05.20.02.42.22 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 May 2016 02:42:22 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-427864-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-427864-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-427864-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=l/7fwDACSoNbgaIH04 Ma7gzvkr5V1DBJvxvN7uOUKJTAQ+Z5I2VVYg4RMaWLlb9MvIeFkREOlZLj46Lkux 5X0boVz4yrWBE5oQiahpMqboXiwU4u5mKKZbSx1hDALagcdioWWVdmtCWP5zRr2o 6n9gYsi8bVtCZBmJzQDVV3cK4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=UMc9RXrvr/TseNexA/KMIbxR ve4=; b=GxWKAlF/FpWU2clK5SoBm15x9ZIw4JtEuG2Nk/++m6mCYScXi8y/Hfpv 3y33DS8KC/z/TKfnnmYKC4wG1jj6yDywsuWWFBgHbK+6/UmtBbYxHgwqlvyRJskN QMw9EljsIXPA3PNw9JFjvY4Twuyn23x/FV7hdxw3OkqXz54aAf0= Received: (qmail 38026 invoked by alias); 20 May 2016 09:42:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 37982 invoked by uid 89); 20 May 2016 09:42:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=among X-HELO: mail-it0-f51.google.com Received: from mail-it0-f51.google.com (HELO mail-it0-f51.google.com) (209.85.214.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 20 May 2016 09:41:54 +0000 Received: by mail-it0-f51.google.com with SMTP id l63so1081415ita.1 for ; Fri, 20 May 2016 02:41:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=QK1LnEWHE0KCsz57o9/OUMtjqWco/O/tF7V2/EijYzk=; b=LAr/3CLdfQWulJ4AmZgwYP3NVliWN5JJvbzBTfEhY0Lr7pH0i6mRr/ZjAyhaIjbf+I wQtsV7zE9syxLsv0nDcBInMhkk0VwQGya63EZr1eMNaSlByBCIlGDM2rHmtEYI77axM6 JnTh45LsYBVxZMUUQqnG9ViEhceDXNidoCkBmLcVhgQUMpqEnjCbChxt+sGjK9UWscix XMUFB+0+AaPBw76LqZ/Z22fNvAmt6En6qsloXHfhUMfX0g0Gh7PfOwP3gnKwFncZx4gF lUsrTC+9xWPkJw09HyvED5LrdVCpNfVqorkMhDhgmk6kiaegIKHnsDnh4tZvW7SDr0cI sMBw== X-Gm-Message-State: AOPr4FXd9e8l1NdslkPTru16Zm96FXgJhQnlLdXl/UflAD5VNmZBwX1rjAm+I10k1qLgE5KOE2n8EEzYBXRa7Ped MIME-Version: 1.0 X-Received: by 10.36.41.79 with SMTP id p76mr2078443itp.31.1463737311742; Fri, 20 May 2016 02:41:51 -0700 (PDT) Received: by 10.36.236.5 with HTTP; Fri, 20 May 2016 02:41:51 -0700 (PDT) In-Reply-To: References: Date: Fri, 20 May 2016 15:11:51 +0530 Message-ID: Subject: Re: increase alignment of global structs in increase_alignment pass From: Prathamesh Kulkarni To: Richard Biener Cc: "gcc@gcc.gnu.org" , gcc Patches X-IsSubscribed: yes On 19 May 2016 at 13:19, Richard Biener wrote: > On Thu, 19 May 2016, Prathamesh Kulkarni wrote: > >> On 18 May 2016 at 19:38, Richard Biener wrote: >> > On Wed, 18 May 2016, Prathamesh Kulkarni wrote: >> > >> >> On 17 May 2016 at 18:36, Richard Biener wrote: >> >> > On Wed, 11 May 2016, Prathamesh Kulkarni wrote: >> >> > >> >> >> On 6 May 2016 at 17:20, Richard Biener wrote: >> >> >> > >> >> >> > You can't simply use >> >> >> > >> >> >> > + offset = int_byte_position (field); >> >> >> > >> >> >> > as it can be placed at variable offset which will make int_byte_position >> >> >> > ICE. Note it also returns a truncated byte position (bit position >> >> >> > stripped) which may be undesirable here. I think you want to use >> >> >> > bit_position and if and only if DECL_FIELD_OFFSET and >> >> >> > DECL_FIELD_BIT_OFFSET are INTEGER_CST. >> >> >> oops, I didn't realize offsets could be variable. >> >> >> Will that be the case only for VLA member inside struct ? >> >> > >> >> > And non-VLA members after such member. >> >> > >> >> >> > Your observation about the expensiveness of the walk still stands I guess >> >> >> > and eventually you should at least cache the >> >> >> > get_vec_alignment_for_record_decl cases. Please make those workers >> >> >> > _type rather than _decl helpers. >> >> >> Done >> >> >> > >> >> >> > You seem to simply get at the maximum vectorized field/array element >> >> >> > alignment possible for all arrays - you could restrict that to >> >> >> > arrays with at least vector size (as followup). >> >> >> Um sorry, I didn't understand this part. >> >> > >> >> > It doesn't make sense to align >> >> > >> >> > struct { int a; int b; int c; int d; float b[3]; int e; }; >> >> > >> >> > because we have a float[3] member. There is no vector size that >> >> > would cover the float[3] array. >> >> Thanks for the explanation. >> >> So we do not want to align struct if sizeof (array_field) < sizeof >> >> (vector_type). >> >> This issue is also present without patch for global arrays, so I modified >> >> get_vec_alignment_for_array_type, to return 0 if sizeof (array_type) < >> >> sizeof (vectype). >> >> > >> >> >> > >> >> >> > + /* Skip artificial decls like typeinfo decls or if >> >> >> > + record is packed. */ >> >> >> > + if (DECL_ARTIFICIAL (record_decl) || TYPE_PACKED (type)) >> >> >> > + return 0; >> >> >> > >> >> >> > I think we should honor DECL_USER_ALIGN as well and not mess with those >> >> >> > decls. >> >> >> Done >> >> >> > >> >> >> > Given the patch now does quite some extra work it might make sense >> >> >> > to split the symtab part out of the vect_can_force_dr_alignment_p >> >> >> > predicate and call that early. >> >> >> In the patch I call symtab_node::can_increase_alignment_p early. I tried >> >> >> moving that to it's callers - vect_compute_data_ref_alignment and >> >> >> increase_alignment::execute, however that failed some tests in vect, and >> >> >> hence I didn't add the following hunk in the patch. Did I miss some >> >> >> check ? >> >> > >> >> > Not sure. >> >> > >> >> >> diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c >> >> >> index 7652e21..2c1acee 100644 >> >> >> --- a/gcc/tree-vect-data-refs.c >> >> >> +++ b/gcc/tree-vect-data-refs.c >> >> >> @@ -795,7 +795,10 @@ vect_compute_data_ref_alignment (struct data_reference *dr) >> >> >> && TREE_CODE (TREE_OPERAND (base, 0)) == ADDR_EXPR) >> >> >> base = TREE_OPERAND (TREE_OPERAND (base, 0), 0); >> >> >> >> >> >> - if (!vect_can_force_dr_alignment_p (base, TYPE_ALIGN (vectype))) >> >> >> + if (!(TREE_CODE (base) == VAR_DECL >> >> >> + && decl_in_symtab_p (base) >> >> >> + && symtab_node::get (base)->can_increase_alignment_p () >> >> >> + && vect_can_force_dr_alignment_p (base, TYPE_ALIGN (vectype)))) >> >> >> { >> >> >> if (dump_enabled_p ()) >> >> >> { >> >> > >> >> > + for (tree field = first_field (type); >> >> > + field != NULL_TREE; >> >> > + field = DECL_CHAIN (field)) >> >> > + { >> >> > + /* Skip if not FIELD_DECL or has variable offset. */ >> >> > + if (TREE_CODE (field) != FIELD_DECL >> >> > + || TREE_CODE (DECL_FIELD_OFFSET (field)) != INTEGER_CST >> >> > + || TREE_CODE (DECL_FIELD_BIT_OFFSET (field)) != INTEGER_CST >> >> > + || DECL_USER_ALIGN (field) >> >> > + || DECL_ARTIFICIAL (field)) >> >> > + continue; >> >> > >> >> > you can stop processing the type and return 0 here if the offset >> >> > is not an INTEGER_CST. All following fields will have the same issue. >> >> > >> >> > + /* FIXME: is this check necessary since we check for variable >> >> > offset above ? */ >> >> > + if (TREE_CODE (offset_tree) != INTEGER_CST) >> >> > + continue; >> >> > >> >> > the check should not be necessary. >> >> > >> >> > offset = tree_to_uhwi (offset_tree); >> >> > >> >> > but in theory offset_tree might not fit a unsigned HOST_WIDE_INT, so >> >> > instead of for INTEGER_CST please check ! tree_fits_uhwi_p (offset_tree). >> >> > As above all following fields will also fail the check if this fails so >> >> > you can return 0 early. >> >> > >> >> > +static unsigned >> >> > +get_vec_alignment_for_type (tree type) >> >> > +{ >> >> > + if (type == NULL_TREE) >> >> > + return 0; >> >> > + >> >> > + gcc_assert (TYPE_P (type)); >> >> > + >> >> > + unsigned *slot = type_align_map->get (type); >> >> > + if (slot) >> >> > + return *slot; >> >> > >> >> > I suggest to apply the caching only for the RECORD_TYPE case to keep >> >> > the size of the map low. >> >> > >> >> > Otherwise the patch looks ok now. >> >> I have done the suggested changes in attached patch. >> >> Is this version OK to commit after bootstrap+test ? >> >> The patch survives cross-testing on arm*-*-* and aarch64*-*-*. >> > >> > + tree type_size_tree = TYPE_SIZE (type); >> > + if (!type_size_tree) >> > + return TYPE_ALIGN (vectype); >> > + >> > + if (TREE_CODE (type_size_tree) != INTEGER_CST) >> > + return TYPE_ALIGN (vectype); >> > + >> > + /* If array has constant size, ensure that it's at least equal to >> > + size of vector type. */ >> > + if (!tree_fits_uhwi_p (type_size_tree)) >> > + return TYPE_ALIGN (vectype); >> > + unsigned HOST_WIDE_INT type_size = tree_to_uhwi (type_size_tree); >> > + >> > + tree vectype_size_tree = TYPE_SIZE (vectype); >> > + if (!tree_fits_uhwi_p (vectype_size_tree)) >> > + return TYPE_ALIGN (vectype); >> > + >> > + unsigned HOST_WIDE_INT vectype_size = tree_to_uhwi (vectype_size_tree); >> > + return (type_size > vectype_size) ? TYPE_ALIGN (vectype) : 0; >> > >> > please change this to >> > >> > if (! TYPE_SIZE (type) >> > || TREE_CODE (TYPE_SIZE (type)) != INTEGER_CST >> > || tree_int_cst_lt (TYPE_SIZE (type), TYPE_SIZE (vectype))) >> > return 0; >> > >> > return TYPE_ALIGN (vectype); >> > >> > consistent with the handling for "VLA" records. >> > >> > + unsigned *slot = type_align_map->get (type); >> > + if (slot) >> > + return *slot; >> > + >> > + unsigned max_align = 0, alignment; >> > + HOST_WIDE_INT offset; >> > + tree offset_tree; >> > + >> > + if (TYPE_PACKED (type)) >> > + return 0; >> > + >> > >> > move the tye_align_map query after the TYPE_PACKED check. >> > >> > + /* We don't need to process the type further if offset is variable, >> > + since the offsets of remaining members will also be variable. */ >> > + if (TREE_CODE (DECL_FIELD_OFFSET (field)) != INTEGER_CST >> > + || TREE_CODE (DECL_FIELD_BIT_OFFSET (field)) != INTEGER_CST) >> > + return 0; >> > >> > just break; here (and in the other case returning zero). If a >> > previous record field said we'd want to align we still should align. >> > Also we want to store 0 into the type_align_map as well. >> > >> > Ok with those changes. >> Hi, >> Is this version OK ? >> Cross tested on arm*-*-* and aarch64*-*-* > > Yes. Btw, please always do a regular bootstrap and regtest, cross-testing > alone is _not_ sufficient! Thanks, bootstrapped and tested on aarch64-linux-gnu, ppc64le-linux-gnu. Unfortunately I screwed up while committing (added the patch but not the test-cases :/). Patch committed in r236502, test-cases committed in r236503. Thanks, Prathamesh > > Thanks, > Richard. > >> Thanks, >> Prathamesh >> > >> > Thanks, >> > Richard. >> > > -- > Richard Biener > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) 2016-05-19 Prathamesh Kulkarni * tree-vectorizer.c (get_vec_alignment_for_decl): New static function. (get_vec_alignment_for_array_decl): Likewise. (get_vec_alignment_for_record_decl): Likewise. (increase_alignment::execute): Move code to find alignment to get_vec_alignment_for_array_decl and call get_vec_alignment_for_decl. (type_align_map): New hash_map. testsuite/ * gcc.dg/vect/section-anchors-vect-70.c: New test-case. * gcc.dg/vect/section-anchors-vect-71.c: Likewise. * gcc.dg/vect/section-anchors-vect-72.c: Likewise.