Hi~
On Wed, May 11, 2022 at 04:53:08PM +0800, Kewen.Lin wrote:
> on 2022/5/10 20:27, Segher Boessenkool wrote:
> > 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.
Iterators of course are *not* just text replacement, which is pretty
nasty, makes inconvenient contentless iterators necessary. Bah. But at
least we can get rid of many attributes.
> >>> +(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).
> >
>
> aha, thanks for the correction and explanation!
I'll commit a patch series doing just this in a bit: it removes <Fv>,
<Ff>, and RS6000_CONSTRAINT_f (it is the same as RS6000_CONSTRAINT_d
always after all, since 2c2aa74d1df0 (2018, removing Xilinx FP)).
> >> 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.
>
> Currently xs{min,max}dp are only covered by smin/smax, no their own
> define_insn?
Yeah, smin/smax cannot work if -ffast-math is not enabled, so we need
some new stuff using an unspec, instead. But ideally we should have an
RTL code that just is the correct thing always! Maybe "fmin/fmax" :-)
Until such a thing exists we can use an unspec, sure.
> Yeah, I agree. The above question/proposal still needs UNSPEC for new
> define_insn, and if we want to replace the expansion pattern for bif
> __builtin_vsx_xs{min,max}dp, we seem to have alternatives:
>
> 1) as Haochen's patch, new define_insn fmin/fmax with UNSPEC_F{MAX,MIN}
> and used for actual insns xs{min,max}dp and replace bif table entry with
> fmin/fmax*.
>
> 2) new define_insn vsx_xs{min,max}dp with UNSPEC_XS{MAX,MIN}DP and used
> for actual insns xs{min,max}dp, new define_expand for fmin/fmax optabs,
> replace bif table entry with vsx_xs{min,max}dp*.
>
> I personally felt (wondered) if 2) is more clear? Because the mnemonics
> xs{min,max}dp don't have fmin/fmax inside the names, ISA doesn't even
> mention they can be used for fmin/fmax in Programming Notes, it seems
> more straight to see vsx_xs{min,max}dp used as bif expansion patterns
> and UNSPEC_XS{MAX,MIN}DP. But both 1) and 2) are fine to me. :)
I prefer 1), for the time being at least, because we have two different
patterns both resulting in xs{min,max}dp.
Segher
