Re: [PATCH ver3] rs6000, Add return value to __builtin_set_fpscr_rn

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

On Tue, 2023-07-11 at 13:54 +0800, Kewen.Lin wrote:
> Hi Carl,
> 
> Excepting for Peter's review comments, some nits are inline below.
> 
> on 2023/7/11 03:18, Carl Love wrote:
> > GCC maintainers:
> > 
> > 
> > 


<snip>

> > -------------------------------------------------
> > rs6000, Add return value  to __builtin_set_fpscr_rn
> 
> Nit: One more unexpected space.

OK, removed

> 
> > Change the return value from void to double for
> > __builtin_set_fpscr_rn.
> > The return value consists of the FPSCR fields DRN, VE, OE, UE, ZE,
> > XE, NI,
> > RN bit positions.  A new test file, test
> > powerpc/test_fpscr_rn_builtin_2.c,
> > is added to test the new return value for the built-in.
> 
> Nit: It would be better to note the newly added
> __SET_FPSCR_RN_RETURNS_FPSCR__
> in commit log as well.

Added a comment as requested.

> 
> > gcc/ChangeLog:
> > 	* config/rs6000/rs6000-builtins.def (__builtin_set_fpscr_rn):
> > Update
> > 	built-in definition return type.
> > 	* config/rs6000-c.cc (rs6000_target_modify_macros): Add check,
> > 	define __SET_FPSCR_RN_RETURNS_FPSCR__ macro.
> > 	* config/rs6000/rs6000.md (rs6000_set_fpscr_rn): Added return
> 
> Nit: s/Added/Add/

Changed.

> 
> > 	argument to return FPSCR fields.
> > 	* doc/extend.texi (__builtin_set_fpscr_rn): Update description
> > for
> > 	the return value.  Add description for
> > 	__SET_FPSCR_RN_RETURNS_FPSCR__ macro.
> > 
> > gcc/testsuite/ChangeLog:
> > 	gcc.target/powerpc/test_fpscr_rn_builtin.c: Renamed to
> > 	test_fpscr_rn_builtin_1.c.  Added comment.
> 
> Nit: s/Added/Add/ and s/Renamed/Rename/.

Changed.

> 
> > 	gcc.target/powerpc/test_fpscr_rn_builtin_2.c: New test for the
> > 	return value of __builtin_set_fpscr_rn builtin.
> > ---
> > 

<snip>

> > -  if (CONST_INT_P (operands[0]))
> > +  /* Emulate the behavior of the mffscrni, mffscrn instructions
> > for earlier
> > +     ISAs.  Return bits 29:31 (DRN) and bits 56:63 (VE, OE, UE,
> > ZE, XE, NI,
> > +     RN) from the FPSCR.  Set the RN field based on the value in
> > operands[1].
> > +  */
> > +
> > +  /* Get the current FPSCR fields, bits 29:31 (DRN) and bits 56:63
> > (VE, OE, UE,
> > +  ZE, XE, NI, RN) from the FPSCR and return them.  */
> > +  rtx tmp_di1 = gen_reg_rtx (DImode);
> 
> Nit: This line is preferred to be move to below (a), close to its
> use.

OK, moved the statement. 
> 
> > +
> > +  emit_insn (gen_rs6000_mffs (tmp_df));
> > +  rtx tmp_di2 = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
> 
> Nit: May be good to rename this tmp_di2 as orig_df_in_di, hope it can
> offer better readablity when people read the code below with its use.

OK, changed the name.  Then changed the name of tmp_di3 to tmp_di2 so
the numbering is sequential.  Moved the new rtx tmp_di2 = gen_reg_rtx
(DImode); right before its use.

> 
> ... (a)
> 
> > +  emit_insn (gen_anddi3 (tmp_di1, tmp_di2, GEN_INT
> > (0x00000007000000FFULL)));
> > +  rtx tmp_rtn = simplify_gen_subreg (DFmode, tmp_di1, DImode, 0);
> > +  emit_move_insn (operands[0], tmp_rtn);
> > +
> > +  if (CONST_INT_P (operands[1]))
> >      {
> > -      if ((INTVAL (operands[0]) & 0x1) == 0x1)
> > +      if ((INTVAL (operands[1]) & 0x1) == 0x1)
> >  	emit_insn (gen_rs6000_mtfsb1 (GEN_INT (31)));
> >        else
> >  	emit_insn (gen_rs6000_mtfsb0 (GEN_INT (31)));
> >  
> > -      if ((INTVAL (operands[0]) & 0x2) == 0x2)
> > +      if ((INTVAL (operands[1]) & 0x2) == 0x2)
> >  	emit_insn (gen_rs6000_mtfsb1 (GEN_INT (30)));
> >        else
> >  	emit_insn (gen_rs6000_mtfsb0 (GEN_INT (30)));
> > @@ -6476,23 +6493,20 @@
> >    else
> >      {
> >        rtx tmp_rn = gen_reg_rtx (DImode);
> > -      rtx tmp_di = gen_reg_rtx (DImode);
> >  
> >        /* Extract new RN mode from operand.  */
> > -      rtx op0 = convert_to_mode (DImode, operands[0], false);
> > -      emit_insn (gen_anddi3 (tmp_rn, op0, GEN_INT (3)));
> > +      rtx op1 = convert_to_mode (DImode, operands[1], false);
> > +      emit_insn (gen_anddi3 (tmp_rn, op1, GEN_INT (3)));
> >  
> > -      /* Insert new RN mode into FSCPR.  */
> > -      emit_insn (gen_rs6000_mffs (tmp_df));
> > -      tmp_di = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
> > -      emit_insn (gen_anddi3 (tmp_di, tmp_di, GEN_INT (-4)));
> > -      emit_insn (gen_iordi3 (tmp_di, tmp_di, tmp_rn));
> > +      /* Insert the new RN value from tmp_rn into FPSCR bit
> > [62:63].  */
> > +      emit_insn (gen_anddi3 (tmp_di1, tmp_di2, GEN_INT (-4)));
> > +      emit_insn (gen_iordi3 (tmp_di1, tmp_di1, tmp_rn));
> >  
> >        /* Need to write to field k=15.  The fields are
> > [0:15].  Hence with
> > -	 L=0, W=0, FLM_i must be equal to 8, 16 = i + 8*(1-W).  FLM is
> > an
> > -	 8-bit field[0:7]. Need to set the bit that corresponds to the
> > -	 value of i that you want [0:7].  */
> > -      tmp_df = simplify_gen_subreg (DFmode, tmp_di, DImode, 0);
> > +         L=0, W=0, FLM_i must be equal to 8, 16 = i + 8*(1-
> > W).  FLM is an
> > +         8-bit field[0:7]. Need to set the bit that corresponds to
> > the
> > +         value of i that you want [0:7].  */
> 
> Nit: Wrong indent change for this comment.  Previous tabs are
> expected.

OK, put tabs back.  Seems emacs wants to use spaces not tabs in .md
files.  I will need to see if I can fix that in the emacs rules.

> 
> > +      tmp_df = simplify_gen_subreg (DFmode, tmp_di1, DImode, 0);
> >        emit_insn (gen_rs6000_mtfsf (GEN_INT (0x01), tmp_df));
> >      }
> >    DONE;
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index cdbd4b34a35..8ff34c33d8e 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -18188,7 +18188,7 @@ double __builtin_mffs (void);
> >  void __builtin_mtfsf (const int, double);
> >  void __builtin_mtfsb0 (const int);
> >  void __builtin_mtfsb1 (const int);
> > -void __builtin_set_fpscr_rn (int);
> > +double __builtin_set_fpscr_rn (int);
> >  @end smallexample
> >  
> >  The @code{__builtin_ppc_get_timebase} and
> > @code{__builtin_ppc_mftb}
> > @@ -18209,13 +18209,20 @@ values to selected fields of the
> > FPSCR.  The
> >  as an argument.  The valid bit range is between 0 and 31.  The
> > builtins map to
> >  the @code{mtfsb0} and @code{mtfsb1} instructions which take the
> > argument and
> >  add 32.  Hence these instructions only modify the FPSCR[32:63]
> > bits by
> > -changing the specified bit to a zero or one respectively.  The
> > -@code{__builtin_set_fpscr_rn} builtin allows changing both of the
> > floating
> > -point rounding mode bits.  The argument is a 2-bit value.  The
> > argument can
> > -either be a @code{const int} or stored in a variable. The builtin
> > uses
> > -the ISA 3.0
> > -instruction @code{mffscrn} if available, otherwise it reads the
> > FPSCR, masks
> > -the current rounding mode bits out and OR's in the new value.
> > +changing the specified bit to a zero or one respectively.
> > +
> > +The @code{__builtin_set_fpscr_rn} built-in allows changing both of
> > the floating
> > +point rounding mode bits and returning the various FPSCR fields
> > before the RN
> > +field is updated.  The built-in returns a double consisting of the
> > initial
> > +value of the FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, and RN bit
> > positions
> > +with all other bits set to zero.  The built-in argument is a 2-bit 
> > value for the
> > +new RN field value.  The argument can either be an @code{const
> > int} or stored
> > +in a variable.  Earlier versions of @code{__builtin_set_fpscr_rn}
> > returned
> > +void.  A @code{__SET_FPSCR_RN_RETURNS_FPSCR__} macro has been
> > added.  If
> > +defined, then the @code{__builtin_set_fpscr_rn} built-in returns
> > the FPSCR
> > +fields.  If not defined, the @code{__builtin_set_fpscr_rn} does
> > not return a
> > +vaule.  If the @option{-msoft-float} option is used, the
> > +@code{__builtin_set_fpscr_rn} built-in will not return a value.
> >  
> >  @node Basic PowerPC Built-in Functions Available on ISA 2.05
> >  @subsubsection Basic PowerPC Built-in Functions Available on ISA
> > 2.05
> > diff --git
> > a/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> > b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_1.c
> > similarity index 89%
> > rename from
> > gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> > rename to
> > gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_1.c
> > index 04707ad8a56..3e22b4e511e 100644
> > --- a/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> > +++ b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_1.c
> > @@ -1,6 +1,12 @@
> > -/* { dg-do run { target { powerpc*-*-* } } } */
> > +/* { dg-do run { target powerpc_fprs } } */
> >  /* { dg-options "-O2 -std=c99" } */
> 
> Nit: Since you remove -std=c99 from _2.c, pls. remove it here to keep
> consistent.

OK, removed.


> 
> >  
> > +/* Originally __builtin_set_fpscr_rn was defined to return
> > void.  It was
> > +   later extended to return a double with the various FPSCR
> > bits.  The
> > +   extended built-in is intended to be a drop in replacement for
> > the original
> > +   version.  This test is for the original version of the built-in 
> > and should
> > +   work exactly as before.  */
> > +
> > 

<snip>

> > +  val = 0x2;
> > +  fpscr_val = wrap_set_fpscr_rn (val);
> > +  check_builtin_set_fpscr_rn (fpscr_val_initial, val, fpscr_val);
> > +  return 0;
> > +
> > +  /* The __SET_FPSCR_RN_RETURNS_FPSCR__ should be defined.  */
> > +  abort();
> 
> Nit: These two lines are useless (unreachable), please remove them.

Argh!  My bad didn't do a very good job of cleanup after removing the
#ifdef __SET_FPSCR_RN_RETURNS_FPSCR__.  Fixed.
> 
> BR,
> Kewen
>