From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67515 invoked by alias); 14 Nov 2016 13:41:13 -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 67504 invoked by uid 89); 14 Nov 2016 13:41:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Mon, 14 Nov 2016 13:41:02 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 10192AAD1; Mon, 14 Nov 2016 13:41:00 +0000 (UTC) Date: Mon, 14 Nov 2016 13:41:00 -0000 From: Richard Biener To: Yuri Rumyantsev cc: Jeff Law , gcc-patches , Ilya Enkovich Subject: Re: [PATCH, vec-tails] Support loop epilogue vectorization 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-11/txt/msg01341.txt.bz2 On Mon, 14 Nov 2016, Yuri Rumyantsev wrote: > Richard, > > In my previous patch I forgot to remove couple lines related to aux field. > Here is the correct updated patch. Yeah, I noticed. This patch would be ok for trunk (together with necessary parts from 1 and 2) if all not required parts are removed (and you'd add the testcases covering non-masked tail vect). Thus, can you please produce a single complete patch containing only non-masked epilogue vectoriziation? Thanks, Richard. > Thanks. > Yuri. > > 2016-11-14 15:51 GMT+03:00 Richard Biener : > > On Fri, 11 Nov 2016, Yuri Rumyantsev wrote: > > > >> Richard, > >> > >> I prepare updated 3 patch with passing additional argument to > >> vect_analyze_loop as you proposed (untested). > >> > >> You wrote: > >> tw, I wonder if you can produce a single patch containing just > >> epilogue vectorization, that is combine patches 1-3 but rip out > >> changes only needed by later patches? > >> > >> Did you mean that I exclude all support for vectorization epilogues, > >> i.e. exclude from 2-nd patch all non-related changes > >> like > >> > >> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c > >> index 11863af..32011c1 100644 > >> --- a/gcc/tree-vect-loop.c > >> +++ b/gcc/tree-vect-loop.c > >> @@ -1120,6 +1120,12 @@ new_loop_vec_info (struct loop *loop) > >> LOOP_VINFO_PEELING_FOR_GAPS (res) = false; > >> LOOP_VINFO_PEELING_FOR_NITER (res) = false; > >> LOOP_VINFO_OPERANDS_SWAPPED (res) = false; > >> + LOOP_VINFO_CAN_BE_MASKED (res) = false; > >> + LOOP_VINFO_REQUIRED_MASKS (res) = 0; > >> + LOOP_VINFO_COMBINE_EPILOGUE (res) = false; > >> + LOOP_VINFO_MASK_EPILOGUE (res) = false; > >> + LOOP_VINFO_NEED_MASKING (res) = false; > >> + LOOP_VINFO_ORIG_LOOP_INFO (res) = NULL; > > > > Yes. > > > >> Did you mean also that new combined patch must be working patch, i.e. > >> can be integrated without other patches? > > > > Yes. > > > >> Could you please look at updated patch? > > > > Will do. > > > > Thanks, > > Richard. > > > >> Thanks. > >> Yuri. > >> > >> 2016-11-10 15:36 GMT+03:00 Richard Biener : > >> > On Thu, 10 Nov 2016, Richard Biener wrote: > >> > > >> >> On Tue, 8 Nov 2016, Yuri Rumyantsev wrote: > >> >> > >> >> > Richard, > >> >> > > >> >> > Here is updated 3 patch. > >> >> > > >> >> > I checked that all new tests related to epilogue vectorization passed with it. > >> >> > > >> >> > Your comments will be appreciated. > >> >> > >> >> A lot better now. Instead of the ->aux dance I now prefer to > >> >> pass the original loops loop_vinfo to vect_analyze_loop as > >> >> optional argument (if non-NULL we analyze the epilogue of that > >> >> loop_vinfo). OTOH I remember we mainly use it to get at the > >> >> original vectorization factor? So we can pass down an (optional) > >> >> forced vectorization factor as well? > >> > > >> > Btw, I wonder if you can produce a single patch containing just > >> > epilogue vectorization, that is combine patches 1-3 but rip out > >> > changes only needed by later patches? > >> > > >> > Thanks, > >> > Richard. > >> > > >> >> Richard. > >> >> > >> >> > 2016-11-08 15:38 GMT+03:00 Richard Biener : > >> >> > > On Thu, 3 Nov 2016, Yuri Rumyantsev wrote: > >> >> > > > >> >> > >> Hi Richard, > >> >> > >> > >> >> > >> I did not understand your last remark: > >> >> > >> > >> >> > >> > That is, here (and avoid the FOR_EACH_LOOP change): > >> >> > >> > > >> >> > >> > @@ -580,12 +586,21 @@ vectorize_loops (void) > >> >> > >> > && dump_enabled_p ()) > >> >> > >> > dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location, > >> >> > >> > "loop vectorized\n"); > >> >> > >> > - vect_transform_loop (loop_vinfo); > >> >> > >> > + new_loop = vect_transform_loop (loop_vinfo); > >> >> > >> > num_vectorized_loops++; > >> >> > >> > /* Now that the loop has been vectorized, allow it to be unrolled > >> >> > >> > etc. */ > >> >> > >> > loop->force_vectorize = false; > >> >> > >> > > >> >> > >> > + /* Add new loop to a processing queue. To make it easier > >> >> > >> > + to match loop and its epilogue vectorization in dumps > >> >> > >> > + put new loop as the next loop to process. */ > >> >> > >> > + if (new_loop) > >> >> > >> > + { > >> >> > >> > + loops.safe_insert (i + 1, new_loop->num); > >> >> > >> > + vect_loops_num = number_of_loops (cfun); > >> >> > >> > + } > >> >> > >> > > >> >> > >> > simply dispatch to a vectorize_epilogue (loop_vinfo, new_loop) > >> >> > >> f> unction which will set up stuff properly (and also perform > >> >> > >> > the if-conversion of the epilogue there). > >> >> > >> > > >> >> > >> > That said, if we can get in non-masked epilogue vectorization > >> >> > >> > separately that would be great. > >> >> > >> > >> >> > >> Could you please clarify your proposal. > >> >> > > > >> >> > > When a loop was vectorized set things up to immediately vectorize > >> >> > > its epilogue, avoiding changing the loop iteration and avoiding > >> >> > > the re-use of ->aux. > >> >> > > > >> >> > > Richard. > >> >> > > > >> >> > >> Thanks. > >> >> > >> Yuri. > >> >> > >> > >> >> > >> 2016-11-02 15:27 GMT+03:00 Richard Biener : > >> >> > >> > On Tue, 1 Nov 2016, Yuri Rumyantsev wrote: > >> >> > >> > > >> >> > >> >> Hi All, > >> >> > >> >> > >> >> > >> >> I re-send all patches sent by Ilya earlier for review which support > >> >> > >> >> vectorization of loop epilogues and loops with low trip count. We > >> >> > >> >> assume that the only patch - vec-tails-07-combine-tail.patch - was not > >> >> > >> >> approved by Jeff. > >> >> > >> >> > >> >> > >> >> I did re-base of all patches and performed bootstrapping and > >> >> > >> >> regression testing that did not show any new failures. Also all > >> >> > >> >> changes related to new vect_do_peeling algorithm have been changed > >> >> > >> >> accordingly. > >> >> > >> >> > >> >> > >> >> Is it OK for trunk? > >> >> > >> > > >> >> > >> > I would have prefered that the series up to -03-nomask-tails would > >> >> > >> > _only_ contain epilogue loop vectorization changes but unfortunately > >> >> > >> > the patchset is oddly separated. > >> >> > >> > > >> >> > >> > I have a comment on that part nevertheless: > >> >> > >> > > >> >> > >> > @@ -1608,7 +1614,10 @@ vect_enhance_data_refs_alignment (loop_vec_info > >> >> > >> > loop_vinfo) > >> >> > >> > /* Check if we can possibly peel the loop. */ > >> >> > >> > if (!vect_can_advance_ivs_p (loop_vinfo) > >> >> > >> > || !slpeel_can_duplicate_loop_p (loop, single_exit (loop)) > >> >> > >> > - || loop->inner) > >> >> > >> > + || loop->inner > >> >> > >> > + /* Required peeling was performed in prologue and > >> >> > >> > + is not required for epilogue. */ > >> >> > >> > + || LOOP_VINFO_EPILOGUE_P (loop_vinfo)) > >> >> > >> > do_peeling = false; > >> >> > >> > > >> >> > >> > if (do_peeling > >> >> > >> > @@ -1888,7 +1897,10 @@ vect_enhance_data_refs_alignment (loop_vec_info > >> >> > >> > loop_vinfo) > >> >> > >> > > >> >> > >> > do_versioning = > >> >> > >> > optimize_loop_nest_for_speed_p (loop) > >> >> > >> > - && (!loop->inner); /* FORNOW */ > >> >> > >> > + && (!loop->inner) /* FORNOW */ > >> >> > >> > + /* Required versioning was performed for the > >> >> > >> > + original loop and is not required for epilogue. */ > >> >> > >> > + && !LOOP_VINFO_EPILOGUE_P (loop_vinfo); > >> >> > >> > > >> >> > >> > if (do_versioning) > >> >> > >> > { > >> >> > >> > > >> >> > >> > please do that check in the single caller of this function. > >> >> > >> > > >> >> > >> > Otherwise I still dislike the new ->aux use and I believe that simply > >> >> > >> > passing down info from the processed parent would be _much_ cleaner. > >> >> > >> > That is, here (and avoid the FOR_EACH_LOOP change): > >> >> > >> > > >> >> > >> > @@ -580,12 +586,21 @@ vectorize_loops (void) > >> >> > >> > && dump_enabled_p ()) > >> >> > >> > dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location, > >> >> > >> > "loop vectorized\n"); > >> >> > >> > - vect_transform_loop (loop_vinfo); > >> >> > >> > + new_loop = vect_transform_loop (loop_vinfo); > >> >> > >> > num_vectorized_loops++; > >> >> > >> > /* Now that the loop has been vectorized, allow it to be unrolled > >> >> > >> > etc. */ > >> >> > >> > loop->force_vectorize = false; > >> >> > >> > > >> >> > >> > + /* Add new loop to a processing queue. To make it easier > >> >> > >> > + to match loop and its epilogue vectorization in dumps > >> >> > >> > + put new loop as the next loop to process. */ > >> >> > >> > + if (new_loop) > >> >> > >> > + { > >> >> > >> > + loops.safe_insert (i + 1, new_loop->num); > >> >> > >> > + vect_loops_num = number_of_loops (cfun); > >> >> > >> > + } > >> >> > >> > > >> >> > >> > simply dispatch to a vectorize_epilogue (loop_vinfo, new_loop) > >> >> > >> > function which will set up stuff properly (and also perform > >> >> > >> > the if-conversion of the epilogue there). > >> >> > >> > > >> >> > >> > That said, if we can get in non-masked epilogue vectorization > >> >> > >> > separately that would be great. > >> >> > >> > > >> >> > >> > I'm still torn about all the rest of the stuff and question its > >> >> > >> > usability (esp. merging the epilogue with the main vector loop). > >> >> > >> > But it has already been approved ... oh well. > >> >> > >> > > >> >> > >> > Thanks, > >> >> > >> > Richard. > >> >> > >> > >> >> > >> > >> >> > > > >> >> > > -- > >> >> > > Richard Biener > >> >> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > >> >> > > >> >> > >> >> > >> > > >> > -- > >> > Richard Biener > >> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > >> > > > > -- > > Richard Biener > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)