Hi guys,
On Mon, May 09, 2022 at 12:05:51PM +0800, Kewen.Lin wrote:
> on 2022/5/9 09:54, HAO CHEN GUI wrote:
> > This patch implements optab f[min/max]_optab by xs[min/max]dp on rs6000.
> > Tests show that outputs of xs[min/max]dp are consistent with the standard
> > of C99 fmin/max.
> > gcc/
> > PR target/103605
> > * rs6000.md (unspec): Add UNSPEC_FMAX and UNSPEC_FMIN.
>
> Nit: one entry for iterator FMINMAX?
It should be
* rs6000.md (FMINMAX): New.
> > (fminmax): New.
> > (minmax_op): Likewise.
Please say "New." when something is new, never "Likewise."
> > (<fminmax><mode>3): New pattern. Implemented by UNSPEC_FMAX and
> > UNSPEC_FMIN.
Leave out the "Implemented..." part please.
> > +(define_int_iterator FMINMAX [UNSPEC_FMAX UNSPEC_FMIN])
> > +
> > +(define_int_attr fminmax [(UNSPEC_FMAX "fmax")
> > + (UNSPEC_FMIN "fmin")])
> > +
> > +(define_int_attr minmax_op [(UNSPEC_FMAX "max")
> > + (UNSPEC_FMIN "min")])
> > +
>
> Can we use the later one for both?
>
> Like f<minmax_op><mode>3.
Yes. RTL iterators and attributes are textual replacement and expansion
only: there is no deeper semantic meaning to it. In fact, we should
have only a "minmax" iterator and a "MINMAX" attribute, and then
simplify everything else. I'll do this for the existing code.
> > +(define_insn "<fminmax><mode>3"
> > + [(set (match_operand:SFDF 0 "vsx_register_operand" "=<Fv>")
> > + (unspec:SFDF [(match_operand:SFDF 1 "vsx_register_operand" "<Fv>")
> > + (match_operand:SFDF 2 "vsx_register_operand" "<Fv>")]
>
> Nit: both SD and DF are mapped to constraint wa, just hardcode Fv to wa?
(SF and DF)
Please keep <Fv> until <Ff> is gone as well, it is easier to read that
way. And at that time get rid of *all* <Fv>.
Indeed we could get rid of it, but only replacing it in some places and
not others is confusing (or at least distracting).
> PR103605 also exposes another problem on bif __builtin_vsx_xs{min,max}dp,
> both bifs are
> expanded into xs{min,max}cdp instead of xs{min,max}dp starting from power9.
>
> IMHO, it's something we want to fix as well, based on the reasons:
> 1) bif names have the corresponding mnemonics, users would expect 1-1
> mapping here.
> 2) clang emits xs{min,max}dp all the time, with cpu type power7/8/9/10.
> 3) according to uarch info, xs{min,max}cdp use the same units and have the
> same latency,
> no benefits to replace with xs{min,max}cdp.
I never understood any of this. Mike? Why do we do those "c" things
at all, ever?
> So I wonder if it would be more clear with:
> 1) add new define_insn for xs{min,max}dp
No, the existing thing should always do these insns.
> 2) use them for new define_expand of fmin/fmax
> 3) use them for bif expansion pattern
There is no way to express fmin/fmax without unspecs currently (without
fastmath). This is a serious problem.
Segher
