Hi Segher,
on 2022/5/10 20:27, Segher Boessenkool wrote:
> 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).
>
aha, thanks for the correction and explanation!
>> 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.
Currently xs{min,max}dp are only covered by smin/smax, no their own define_insn?
>
>> 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.
>
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. :)
BR,
Kewen
