From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58682 invoked by alias); 21 Sep 2016 07:16:06 -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 58148 invoked by uid 89); 21 Sep 2016 07:16:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=goups_size, Moved 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 ESMTP; Wed, 21 Sep 2016 07:16:04 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D0029AAD1; Wed, 21 Sep 2016 07:16:01 +0000 (UTC) Date: Wed, 21 Sep 2016 07:27:00 -0000 From: Richard Biener To: Bernd Edlinger cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH] Fix PR tree-optimization/77550 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2016-09/txt/msg01380.txt.bz2 On Tue, 20 Sep 2016, Bernd Edlinger wrote: > On 09/20/16 09:41, Richard Biener wrote: > > On Mon, 19 Sep 2016, Bernd Edlinger wrote: > > > >> On 09/19/16 11:25, Richard Biener wrote: > >>> On Sun, 18 Sep 2016, Bernd Edlinger wrote: > >>> > >>>> Hi, > >>>> > >>>> this PR shows that in vectorizable_store and vectorizable_load > >>>> as well, the vector access always uses the first dr as the alias > >>>> type for the whole access. But that is not right, if they are > >>>> different types, like in this example. > >>>> > >>>> So I tried to replace all reference_alias_ptr_type (DR_REF (first_dr)) > >>>> by an alias type that is correct for all references in the whole > >>>> access group. With this patch we fall back to ptr_type_node, which > >>>> can alias anything, if the group consists of different alias sets. > >>>> > >>>> > >>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > >>>> Is it OK for trunk and gcc-6-branch? > >>> > >>> +/* Function get_group_alias_ptr_type. > >>> + > >>> + Return the alias type for the group starting at FIRST_STMT > >>> + containing GROUP_SIZE elements. */ > >>> + > >>> +static tree > >>> +get_group_alias_ptr_type (gimple *first_stmt, int group_size) > >>> +{ > >>> + struct data_reference *first_dr, *next_dr; > >>> + gimple *next_stmt; > >>> + int i; > >>> + > >>> + first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt)); > >>> + next_stmt = GROUP_NEXT_ELEMENT (vinfo_for_stmt (first_stmt)); > >>> + for (i = 1; i < group_size && next_stmt; i++) > >>> + { > >>> > >>> > >>> there is no need to pass in group_size, it's enough to walk > >>> GROUP_NEXT_ELEMENT until it becomes NULL. > >>> > >>> Ok with removing the redundant arg. > >>> > >>> Thanks, > >>> Richard. > >>> > >> > >> Hmmm, I'm afraid this needs one more iteration. > >> > >> > >> I tried first to check if there are no stmts after the group_size > >> and noticed there are cases when group_size is 0, > >> for instance in gcc.dg/torture/pr36244.c. > >> > >> I think there is a bug in vectorizable_load, here: > >> > >> if (grouped_load) > >> { > >> first_stmt = GROUP_FIRST_ELEMENT (stmt_info); > >> /* For SLP vectorization we directly vectorize a subchain > >> without permutation. */ > >> if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ()) > >> first_stmt = ; > >> > >> group_size = GROUP_SIZE (vinfo_for_stmt (first_stmt)); > >> > >> = 0, and even worse: > >> > >> group_gap_adj = vf * group_size - nunits * vec_num; > >> > >> = -4 ! > >> > >> apparently GROUP_SIZE is only valid on the GROUP_FIRST_ELEMENT, > > > > Yes. I'm not sure group_size or group_gap_adj are used in the > > slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists () case but moving > > the computation up before we re-set first_stmt is probably a good idea. > > > >> while it may be 0 on SLP_TREE_SCALAR_STMTS (slp_node)[0] > >> > >> moving the GROUP_SIZE up before first_stmt is overwritten > >> results in no different code.... > > > > See above - it's eventually unused. The load/store vectorization code > > is quite twisted ;) > > > > Agreed. > > Here is the new version of the patch: > > Moved the goups_size up, and everything works fine. > > Removed the parameter from get_group_alias_ptr_type. > > I think in the case, where first_stmt is not set to > GROUP_FIRST_ELEMENT (stmt_info) but directly to stmt, > it is likely somewhere in a list, so it is not necessary > to walk the GROUP_NEXT_ELEMENT, so I would like to call > reference_alias_ptr_type directly in that case. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk and gcc-6 branch? Ok for trunk and gcc-6 branch after a while. Richard.