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

[Carl Love <cel@us.ibm.com>]

On Mon, 2023-05-22 at 14:36 +0800, Kewen.Lin wrote:
> 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 builtins were requested for use in GLibC.  As of version 2.31 they
were added as inline asm.  They requested a builtin so the asm could be
removed.

> 
> > 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/

fixed
> 
> > 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/

fixed

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

Moved them to power 9 stanza.

>   Besides, {nosoft} attribute is required.

OK, added that.  I was trying to figure out why nosoft is needed.  The
instructions are manipulating bits in a physical register that controls
the hardware floating point instructions.  It looks to me like that
would be why.  Because if you were using msoft float then the floating
point HW registers are disabled and the floating point operations are
done using software.  Did I figure this out correctly?

 
> 
> >  ; 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/

Fixed

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

I think it should be sufficient to just support these when floating
hardware instructions are supported.  So I believe I just need to move
these to the correct place in the document.  I think that means they
should be in the section:

  The following functions require @option{-mhard-float},
  @option{-mpowerpc-gfxopt}, and @option{-mpopcntb} options.

Moved to the end of the above section.  Hope that is correct.

> 
> > +
> >  @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."

Updated the description to not specify the specific bit positions.

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

changed

> 
> > +/* { dg-options "-mpower9-vector -mdejagnu-cpu=power9" } */
> 
> ... and it doesn't need -mpower9-vector here.

Removed -mpower9-vector
> 
> BR,
> Kewen
>