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
> 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.
> 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.
> * config/rs6000/rs6000.md ((rs6000_get_fpscr_fields): New
> define_expand.
Too many '('.
> (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.
> * 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/
> + /* 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.
> +(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.
> +(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.
> +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).
> +/* 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.
> +++ 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.
Peter
