Re: [PING][Patch 3/3][Arm] Add support for IEEE-conformant versions of scalar fmin* and fmax*

[Alan Lawrence <alan dot lawrence at foss dot arm dot com>]

On 16/12/15 09:31, Kyrill Tkachov wrote:
> Hi David,
>
> On 16/12/15 08:53, David Sherwood wrote:
> > Hi,
> >
> > Here is the last patch of the fmin/fmax change, which adds the optabs
> > to the arm backend.
> >
> > Tested:
> >
> > arm-none-eabi: no regressions
> >
> > Good to go?
> > David Sherwood.
> >
> > ChangeLog:
> >
> > 2015-12-08  David Sherwood  <david.sherwood@arm.com>
> >
> >      gcc/
> >          * config/arm/iterators.md: New iterators.
> >          * config/arm/unspecs.md: New unspecs.
> >          * config/arm/neon.md: New pattern.
> >          * config/arm/vfp.md: Likewise.
>
> Please list the new entities you add in parentheses.
> For example:
>      * config/arm/iterators.md (VMAXMINFNM): New int iterator.
>      (fmaxmin): New int attribute.
>      (fmaxmin): Likewise.
>
> Same for the other files. That way one can grep through the ChangeLogs to
> find when any particular pattern/iterator/whatever was modified.
>
> +;; Auto-vectorized forms for the IEEE-754 fmax()/fmin() functions
> +(define_insn "<fmaxmin><mode>3"
> +  [(set (match_operand:VCVTF 0 "s_register_operand" "=w")
> +    (unspec:VCVTF [(match_operand:VCVTF 1 "s_register_operand" "w")
> +               (match_operand:VCVTF 2 "s_register_operand" "w")]
> +               VMAXMINFNM))]
> +  "TARGET_NEON && TARGET_FPU_ARMV8"
> +  "<fmaxmin_op>.<V_s_elem>\t%<V_reg>0, %<V_reg>1, %<V_reg>2"
> +  [(set_attr "type" "neon_fp_minmax_s<q>")]
> +)
>
> I would just say "Vector forms" rather than "Auto-vectorized".
> In principle we can get vector types through other means besides
> auto-vectorisation.
>
> +;; Scalar forms for the IEEE-754 fmax()/fmin() functions
> +(define_insn "<fmaxmin><mode>3"
> +  [(set (match_operand:SDF 0 "s_register_operand" "=<F_constraint>")
> +    (unspec:SDF [(match_operand:SDF 1 "s_register_operand" "<F_constraint>")
> +             (match_operand:SDF 2 "s_register_operand" "<F_constraint>")]
> +             VMAXMINFNM))]
> +  "TARGET_HARD_FLOAT && TARGET_VFP5 <vfp_double_cond>"
> +  "<fmaxmin_op>.<V_if_elem>\\t%<V_reg>0, %<V_reg>1, %<V_reg>2"
> +  [(set_attr "type" "f_minmax<vfp_type>")
> +   (set_attr "conds" "unconditional")]
> +)
> +
>
> I notice your new test doesn't test the SF variant of this.
> Could you please add something to test it?
> Looks good to me otherwise.
>
> Thanks,
> Kyrill

I note strong similarities between this patch and Bilyan Borisov's 
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01971.html

Both add the same UNSPEC_s, and equivalent VMAXMIN(F?)NM. David's adds <fmaxmin> 
and <fmaxmin_op> attributes, whereas Bilyan reuses some elements of the existing 
<maxmin>. AFAICT, the patterns they add are in other ways equivalent (same type, 
condition, modes, alternatives), albeit in different files and constructed using 
those different iterators, and David's has the standard names (which IIUC we 
want, so the autovectorizer finds them) whereas Bilyan adds the intrinsics 
(which we also want)...

--Alan