Re: [PATCH, rs6000] Implement mffscrni pattern

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Mon, Dec 20, 2021 at 01:55:51PM +0800, HAO CHEN GUI wrote:
> 	* config/rs6000/rs6000-call.c
> 	(rs6000_expand_set_fpscr_rn_builtin): Not copy argument to a reg if
> 	it's a constant. The pattern for constant can be recognized now.

(Two spaces after full stop).

> +;; Return 1 if op is an unsigned 2-bit constant integer.
> +(define_predicate "u2bit_cint_operand"
> +  (and (match_code "const_int")
> +       (match_test "INTVAL (op) >= 0 && INTVAL (op) <= 3")))

Simple project for anyone: use IN_RANGE more :-)

> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -177,6 +177,7 @@ (define_c_enum "unspecv"
>     UNSPECV_MFFS			; Move from FPSCR
>     UNSPECV_MFFSL		; Move from FPSCR light instruction version
>     UNSPECV_MFFSCRN		; Move from FPSCR float rounding mode
> +   UNSPECV_MFFSCRNI		; Move from FPSCR float rounding mode with imm

Why can this not just use the existing UNSPECV_MFFSCRN?  That way, an
mffsrcn can automatically morph into an mffscrni if the argument is a
constant integer, etc.

> @@ -6333,9 +6342,15 @@ (define_expand "rs6000_set_fpscr_rn"
>       new rounding mode bits from operands[0][62:63] into FPSCR[62:63].  */
>    if (TARGET_P9_MISC)
>      {
> -      rtx src_df = force_reg (DImode, operands[0]);
> -      src_df = simplify_gen_subreg (DFmode, src_df, DImode, 0);
> -      emit_insn (gen_rs6000_mffscrn (tmp_df, src_df));
> +      if (CONST_INT_P (operands[0])
> +	  && const_0_to_3_operand (operands[0], VOIDmode))

const_0_to_3_operand already checks that CONST_INT_P is true (so you
do not need to test the latter separately).

> @@ -6357,7 +6372,8 @@ (define_expand "rs6000_set_fpscr_rn"
>        rtx tmp_di = gen_reg_rtx (DImode);
> 
>        /* Extract new RN mode from operand.  */
> -      emit_insn (gen_anddi3 (tmp_rn, operands[0], GEN_INT (0x3)));
> +      rtx op0 = convert_to_mode (DImode, operands[0], false);
> +      emit_insn (gen_anddi3 (tmp_rn, op0, GEN_INT (0x3)));

Please write 3 as 3.  Not as 0x0003, not as 0x03, not as 0x3.  I realise
you just copied this, but :-)

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/mffscrni_p9.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile { target { has_arch_pwr9 } } } */

Don't do that?  Instead, use -mdejagnu-cpu=power9.  This will give more
coverage, and also will make us need less future test maintenance (if on
Power28 we will generate different code for this, for example).

> +void __attribute__ ((noinline)) wrap_set_fpscr_rn (int val)
> +{
> +  __builtin_set_fpscr_rn (val);
> +}

Should be noipa, not just noinline?


Segher