Re: [PATCH, vec-tails] Support loop epilogue vectorization

[Yuri Rumyantsev <ysrumyan at gmail dot com>]

Richard,

Here is fixed version of updated patch 3.

Any comments will be appreciated.

Thanks.
Yuri.

2016-11-11 17:15 GMT+03:00 Yuri Rumyantsev <ysrumyan@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 <ysrumyan@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 <rguenther@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 <rguenther@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 <rguenther@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 <rguenther@suse.de>
>>>> > > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
>>>> >
>>>>
>>>>
>>>
>>> --
>>> Richard Biener <rguenther@suse.de>
>>> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Attachment: patch.03.update1
Description: Binary data