On Thu, 2023-07-06 at 17:54 -0500, Peter Bergner wrote:
> On 6/30/23 7:58 PM, Carl Love via Gcc-patches wrote:
> > rs6000, __builtin_set_fpscr_rn add retrun value
>
> s/retrun/return/
>
> Maybe better written as:
>
> rs6000: Add return value to __builtin_set_fpscr_rn
Changed subject, fixed misspelling.
>
>
> > Change the return value from void to double. The return value
> > consists of
> > the FPSCR fields DRN, VE, OE, UE, ZE, XE, NI, RN bit
> > positions. Add an
> > overloaded version which accepts a double argument.
>
> You're not adding an overloaded version anymore, so I think you can
> just
> remove the last sentence.
Yup, didn't get that removed when removing the overloaded instance.
fixed.
>
>
>
> > The test powerpc/test_fpscr_rn_builtin.c is updated to add tests
> > for the
> > double reterun value and the new double argument.
>
> s/reterun/return/ ...and there is no double argument anymore, so
> that
> part can be removed.
Fixed. Note, the new return value tests were moved to new test file.
>
>
>
> > * config/rs6000/rs6000.md ((rs6000_get_fpscr_fields): New
> > define_expand.
>
> Too many '('.
fixed.
>
>
>
> > (rs6000_set_fpscr_rn): Added return argument. Updated
> > to use new
>
> Looks like a <tab> after Added instead of a space.
>
>
> > rs6000_get_fpscr_fields and rs6000_update_fpscr_rn_field define
> > _expands.
>
> Don't split define_expand across two lines.
Fixed.
>
>
>
> > * doc/extend.texi (__builtin_set_fpscr_rn): Update description
> > for
> > the return value and new double argument. Add descripton for
> > __SET_FPSCR_RN_RETURNS_FPSCR__ macro.
>
> s/descripton/description/
Fixed.
>
>
>
>
>
>
> > + /* Tell the user the __builtin_set_fpscr_rn now returns the
> > FPSCR fields
> > + in a double. Originally the builtin returned void. */
>
> Either:
> 1) s/Tell the user the __builtin_set_fpscr_rn/Tell the user
> __builtin_set_fpscr_rn/
> 2) s/the __builtin_set_fpscr_rn now/the __builtin_set_fpscr_rn
> built-in now/
>
>
> > + if ((flags & OPTION_MASK_SOFT_FLOAT) == 0)
> > + rs6000_define_or_undefine_macro (define_p,
> > "__SET_FPSCR_RN_RETURNS_FPSCR__");
>
> This doesn't look like it's indented correctly.
>
>
Fixed indentation.
>
>
> > +(define_expand "rs6000_get_fpscr_fields"
> > + [(match_operand:DF 0 "gpc_reg_operand")]
> > + "TARGET_HARD_FLOAT"
> > +{
> > + /* Extract fields bits 29:31 (DRN) and bits 56:63 (VE, OE, UE,
> > ZE, XE, NI,
> > + RN) from the FPSCR and return them. */
> > + rtx tmp_df = gen_reg_rtx (DFmode);
> > + rtx tmp_di = gen_reg_rtx (DImode);
> > +
> > + 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
> > (0x00000007000000FFULL)));
> > + rtx tmp_rtn = simplify_gen_subreg (DFmode, tmp_di, DImode, 0);
> > + emit_move_insn (operands[0], tmp_rtn);
> > + DONE;
> > +})
>
> This doesn't look correct. You first set tmp_di to a new reg rtx but
> then
> throw that away with the return value of simplify_gen_subreg(). I'm
> guessing
> you want that tmp_di as a gen_reg_rtx for the destination of the
> gen_anddi3, so
> you probably want a different rtx for the subreg that feeds the
> gen_anddi3.
OK, fixed the use of the tmp values. Note the define_expand was
inlined into define_expand "rs6000_set_fpscr_rn per comments from
Kewen. Inlining allows the reuse some of the tmp values.
>
>
>
> > +(define_expand "rs6000_update_fpscr_rn_field"
> > + [(match_operand:DI 0 "gpc_reg_operand")]
> > + "TARGET_HARD_FLOAT"
> > +{
> > + /* Insert the new RN value from operands[0] into FPSCR bit
> > [62:63]. */
> > + rtx tmp_di = gen_reg_rtx (DImode);
> > + rtx tmp_df = gen_reg_rtx (DFmode);
> > +
> > + emit_insn (gen_rs6000_mffs (tmp_df));
> > + tmp_di = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
>
> Ditto.
Fixed.
>
>
>
>
> > +The @code{__builtin_set_fpscr_rn} builtin allows changing both of
> > the floating
> > +point rounding mode bits and returning the various FPSCR fields
> > before the RN
> > +field is updated. The builtin 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 builtin 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} builtin 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} builtin
> > +will not return a value.
>
> Multiple occurrences of "builtin" that should be spelled "built-in"
> (not in the
> built-in function name itself though).
Went thru the patch to fix the spelling of built-in.
>
>
>
> > +/* Originally the __builtin_set_fpscr_rn builtin was defined to
> > return
> > + void. It was later extended to return a double with the
> > various
> > + FPSCR bits. The extended builtin is inteded to be a drop in
> > replacement
> > + for the original version. This test is for the original
> > version of the
> > + builtin and should work exactly as before. */
>
> Ditto.
Fixed.
>
>
>
>
> > +++ b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin_2.c
> > @@ -0,0 +1,153 @@
> > +/* { dg-do run { target { powerpc*-*-* } } } */
>
> powerpc*-*-* is the default for this test directory, so you can drop
> that,
> but you need to disable this test for soft-float systems, so you
> probably want:
>
> /* { dg-do run { target powerpc_fprs } } */
>
> I know you didn't write it, but test_fpscr_rn_builtin_1.c should be
> changed to
> use the same dg-do run test above as well.
Fixed in both test files.
Carl
