Re: [PATCH, rs6000] Add expansions for min/max vector reductions

[Bill Schmidt <wschmidt at linux dot vnet dot ibm dot com>]

On Wed, 2015-09-16 at 18:03 +0100, Alan Lawrence wrote:
> On 16/09/15 17:10, Bill Schmidt wrote:
> >
> > On Wed, 2015-09-16 at 16:29 +0100, Alan Lawrence wrote:
> >> On 16/09/15 15:28, Bill Schmidt wrote:
> >>> 2015-09-16  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> >>>
> >>>           * config/rs6000/altivec.md (UNSPEC_REDUC_SMAX, UNSPEC_REDUC_SMIN,
> >>>           UNSPEC_REDUC_UMAX, UNSPEC_REDUC_UMIN, UNSPEC_REDUC_SMAX_SCAL,
> >>>           UNSPEC_REDUC_SMIN_SCAL, UNSPEC_REDUC_UMAX_SCAL,
> >>>           UNSPEC_REDUC_UMIN_SCAL): New enumerated constants.
> >>>           (reduc_smax_v2di): New define_expand.
> >>>           (reduc_smax_scal_v2di): Likewise.
> >>>           (reduc_smin_v2di): Likewise.
> >>>           (reduc_smin_scal_v2di): Likewise.
> >>>           (reduc_umax_v2di): Likewise.
> >>>           (reduc_umax_scal_v2di): Likewise.
> >>>           (reduc_umin_v2di): Likewise.
> >>>           (reduc_umin_scal_v2di): Likewise.
> >>>           (reduc_smax_v4si): Likewise.
> >>>           (reduc_smin_v4si): Likewise.
> >>>           (reduc_umax_v4si): Likewise.
> >>>           (reduc_umin_v4si): Likewise.
> >>>           (reduc_smax_v8hi): Likewise.
> >>>           (reduc_smin_v8hi): Likewise.
> >>>           (reduc_umax_v8hi): Likewise.
> >>>           (reduc_umin_v8hi): Likewise.
> >>>           (reduc_smax_v16qi): Likewise.
> >>>           (reduc_smin_v16qi): Likewise.
> >>>           (reduc_umax_v16qi): Likewise.
> >>>           (reduc_umin_v16qi): Likewise.
> >>>           (reduc_smax_scal_<mode>): Likewise.
> >>>           (reduc_smin_scal_<mode>): Likewise.
> >>>           (reduc_umax_scal_<mode>): Likewise.
> >>>           (reduc_umin_scal_<mode>): Likewise.
> >>
> >> You shouldn't need the non-_scal reductions. Indeed, they shouldn't be used if
> >> the _scal are present. The non-_scal's were previously defined as producing a
> >> vector with one element holding the result and the other elements all zero, and
> >> this was only ever used with a vec_extract immediately after; the _scal pattern
> >> now includes the vec_extract as well. Hence the non-_scal patterns are
> >> deprecated / considered legacy, as per md.texi.
> >
> > Thanks -- I had misread the description of the non-scalar versions,
> > missing the part where the other elements are zero.  What I really
> > want/need is an optab defined as computing the maximum value in all
> > elements of the vector.
> 
> Yes, indeed. It seems reasonable to me that this would coexist with an optab 
> which computes only a single value (i.e. a scalar).
> 
> At that point it might be appropriate to change the cond-reduction code to 
> generate the reduce-to-vector in all cases, and optabs.c expand it to 
> reduce-to-scalar + broadcast if reduce-to-vector was not available. Along with 
> the (parallel) changes to cost model already proposed, does that cover all the 
> cases? It does add a new tree code, yes, but I'm feeling that could be justified 
> if we go down this route.

That's how I envisioned it as well, and it was my original preference,
provided the maintainers are ok with it.  However, your next suggestion
is intriguing...

> 
> However, another point that springs to mind: if you reduce a loop containing OR 
> or MUL expressions, the vect_create_epilog_for_reduction reduces these using 
> shifts, and I think will also use shifts for platforms not possessing a 
> reduc_plus/min/max. If shifts could be changed for rotates, the code there would 
> do your reduce-to-a-vector-of-identical-elements in the midend...can we 
> (sensibly!) bring all of these together?

Perhaps so.  I can have a look at that and see.  What I'm calling a
rotate is really a double-vector shift where both input vectors are the
same, so perhaps this is already pretty close to what we need.

Thanks!  I'll try to put together some sort of proposal over the next
week or so, workload permitting.

Bill

> 
> > Perhaps the practical thing is to have the vectorizer also do an
> > add_stmt_cost with some new token that indicates the cost model should
> > make an adjustment if the back end doesn't need the extract/broadcast.
> > Targets like PowerPC and AArch32 could then subtract the unnecessary
> > cost, and remove the unnecessary code in simplify-rtx.
> 
> I think it'd be good if we could do it before simplify-rtx, really, although I'm 
> not sure I have a strong argument as to why, as long as we can cost it 
> appropriately.
> 
> > In any case, I will remove implementing the deprecated optabs, and I'll
> > also try to look at Alan L's patch shortly.
> 
> That'd be great, thanks :)
> 
> Cheers, Alan
>