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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

On 22/12/15 15:15, Alan Lawrence wrote:
> 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

You're right.

> 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)...

So what I think we want is to take Davids' patch (after my earlier comments are addressed)
and rebase Bilyans' patch on top of that to add the intrinsics and ACLE macro (and the tests for these, of course)

Thanks,
Kyrill

> --Alan