Re: [PATCH v2] rs6000: Add buildin for mffscrn instructions

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Carl,

on 2023/5/19 05:12, Carl Love via Gcc-patches wrote:
> GCC maintainers:
> 
> version 2.  Fixed an issue with the test case.  The dg-options line was
> missing.
> 
> The following patch adds an overloaded builtin.  There are two possible
> arguments for the builtin.  The builtin definitions are:
> 
>   double __builtin_mffscrn (unsigned long int);
>   double __builtin_mffscrn (double);
> 

We already have one  bif __builtin_set_fpscr_rn for RN setting, apparently
these two are mainly for direct mapping to mffscr[ni] and want the FPSCR bits.
I'm curious what's the requirements requesting these two built-in functions?

> The patch has been tested on Power 10 with no regressions.  
> 
> Please let me know if the patch is acceptable for mainline.  Thanks.
> 
>                     Carl
> 
> ------------------------------------------------
> rs6000: Add buildin for mffscrn instructions
> 

s/buildin/built-in/

> This patch adds overloaded __builtin_mffscrn for the move From FPSCR
> Control & Set R instruction with an immediate argument.  It also adds the
> builtin with a floating point register argument.  A new runnable test is
> added for the new builtin.

s/Set R/Set RN/

> 
> gcc/
> 
> 	* config/rs6000/rs6000-builtins.def (__builtin_mffscrni,
> 	__builtin_mffscrnd): Add builtin definitions.
> 	* config/rs6000/rs6000-overload.def (__builtin_mffscrn): Add
> 	overloaded definition.
> 	* doc/extend.texi: Add documentation for __builtin_mffscrn.
> 
> gcc/testsuite/
> 
> 	* gcc.target/powerpc/builtin-mffscrn.c: Add testcase for new
> 	builtin.
> ---
>  gcc/config/rs6000/rs6000-builtins.def         |   7 ++
>  gcc/config/rs6000/rs6000-overload.def         |   5 +
>  gcc/doc/extend.texi                           |   8 ++
>  .../gcc.target/powerpc/builtin-mffscrn.c      | 106 ++++++++++++++++++
>  4 files changed, 126 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-mffscrn.c
> 
> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
> index 92d9b46e1b9..67125473684 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -2875,6 +2875,13 @@
>    pure vsc __builtin_vsx_xl_len_r (void *, signed long);
>      XL_LEN_R xl_len_r {}
>  
> +; Immediate instruction only uses the least significant two bits of the
> +; const int.
> +  double __builtin_mffscrni (const int<2>);
> +    MFFSCRNI rs6000_mffscrni {}
> +
> +  double __builtin_mffscrnd (double);
> +    MFFSCRNF rs6000_mffscrn {}
>  

Why are them put in [power9-64] rather than [power9]?  IMHO [power9] is the
correct stanza for them.  Besides, {nosoft} attribute is required.

>  ; Builtins requiring hardware support for IEEE-128 floating-point.
>  [ieee128-hw]
> diff --git a/gcc/config/rs6000/rs6000-overload.def b/gcc/config/rs6000/rs6000-overload.def
> index c582490c084..adda2df69ea 100644
> --- a/gcc/config/rs6000/rs6000-overload.def
> +++ b/gcc/config/rs6000/rs6000-overload.def
> @@ -78,6 +78,11 @@
>  ; like after a required newline, but nowhere else.  Lines beginning with
>  ; a semicolon are also treated as blank lines.
>  
> +[MFFSCR, __builtin_mffscrn, __builtin_mffscrn]
> +  double __builtin_mffscrn (const int<2>);
> +    MFFSCRNI
> +  double __builtin_mffscrn (double);
> +    MFFSCRNF
>  
>  [BCDADD, __builtin_bcdadd, __builtin_vec_bcdadd]
>    vsq __builtin_vec_bcdadd (vsq, vsq, const int);
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index ed8b9c8a87b..f16c046051a 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -18455,6 +18455,9 @@ int __builtin_dfp_dtstsfi_ov_td (unsigned int comparison, _Decimal128 value);
>  
>  double __builtin_mffsl(void);
>  
> +double __builtin_mffscrn (unsigned long int);
> +double __builtin_mffscrn (double);

s/unsigned long int/const int/

Note that this section is for all configurations and your implementation is put
__builtin_mffscrn power9 only, so if the intention (requirement) is to make this
be for also all configurations, we need to deal with the cases without the support
of actual hw insns mffscrn{,i}, just like the existing handlings for mffsl etc.

> +
>  @end smallexample
>  The @code{__builtin_byte_in_set} function requires a
>  64-bit environment supporting ISA 3.0 or later.  This function returns
> @@ -18511,6 +18514,11 @@ the FPSCR.  The instruction is a lower latency version of the @code{mffs}
>  instruction.  If the @code{mffsl} instruction is not available, then the
>  builtin uses the older @code{mffs} instruction to read the FPSCR.
>  
> +The @code{__builtin_mffscrn} returns the contents of the control bits in the
> +FPSCR, bits 29:31 (DRN) and bits 56:63 (VE, OE, UE, ZE, XE, NI, RN).  The
> +contents of bits [62:63] of the unsigned long int or double argument are placed
> +into bits [62:63] of the FPSCR (RN).
> +

I know this description is copied from ISA doc, but this part is for GCC documentation,
the document conventions can be different, I prefer to just describe which control
bits in FPSCR like "control bits DRN and VE, OE, UE, ZE, XE, NI, RN in FPSCR are
returned with the same locations in FPSCR, RN in FPSCR is set according to the given
2-bits constant for the variant with const int type argument, or the given double whose
two bits sits in the same locations of FPSCR RN for the variant with double type argument."

>  @node Basic PowerPC Built-in Functions Available on ISA 3.1
>  @subsubsection Basic PowerPC Built-in Functions Available on ISA 3.1
>  
> diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-mffscrn.c b/gcc/testsuite/gcc.target/powerpc/builtin-mffscrn.c
> new file mode 100644
> index 00000000000..26c666a4091
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-mffscrn.c
> @@ -0,0 +1,106 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target p9vector_hw } */

Since it's nothing about vector, I think you should use p9modulo_hw instead ...

> +/* { dg-options "-mpower9-vector -mdejagnu-cpu=power9" } */

... and it doesn't need -mpower9-vector here.

BR,
Kewen