On Fri, 11 Nov 2016, Yuri Rumyantsev wrote:
> Richard,
>
> Here is fixed version of updated patch 3.
>
> Any comments will be appreciated.
Looks good apart from
+ if (epilogue)
+ {
+ epilogue->force_vectorize = loop->force_vectorize;
+ epilogue->safelen = loop->safelen;
+ epilogue->dont_vectorize = false;
+
+ /* We may need to if-convert epilogue to vectorize it. */
+ if (LOOP_VINFO_SCALAR_LOOP (loop_vinfo))
+ tree_if_conversion (epilogue);
+
+ gcc_assert (!epilogue->aux);
+ epilogue->aux = loop_vinfo;
where the last two lines should now no longer be necessary?
Thanks,
Richard.
> Thanks.
> Yuri.
>
> 2016-11-11 17:15 GMT+03:00 Yuri Rumyantsev <ysrum...@gmail.com>:
> > Richard,
> >
> > Sorry for confusion but my updated patch does not work properly, so I
> > need to fix it.
> >
> > Yuri.
> >
> > 2016-11-11 14:15 GMT+03:00 Yuri Rumyantsev <ysrum...@gmail.com>:
> >> 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;
> >>
> >> Did you mean also that new combined patch must be working patch, i.e.
> >> can be integrated without other patches?
> >>
> >> Could you please look at updated patch?
> >>
> >> Thanks.
> >> Yuri.
> >>
> >> 2016-11-10 15:36 GMT+03:00 Richard Biener <rguent...@suse.de>:
> >>> 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 <rguent...@suse.de>:
> >>>> > > 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 <rguent...@suse.de>:
> >>>> > >> > 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 <rguent...@suse.de>
> >>>> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham
> >>>> > > Norton, HRB 21284 (AG Nuernberg)
> >>>> >
> >>>>
> >>>>
> >>>
> >>> --
> >>> Richard Biener <rguent...@suse.de>
> >>> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton,
> >>> HRB 21284 (AG Nuernberg)
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
