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
>
