[rs6000] Implement mffscrni pattern

  Hi,
  I modified the patch according to David and Segher's advice.

  This patch defines a pattern for mffscrni. If the RN is a constant, it can call
gen_rs6000_mffscrni directly. The "rs6000-builtin-new.def" defines prototype for builtin arguments.
The pattern "rs6000_set_fpscr_rn" is then broken as the mode of its argument is DI while its
corresponding builtin has a const int argument. The patch also fixed it.

  Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk?
Any recommendations? Thanks a lot.

ChangeLog
2021-12-17 Haochen Gui <guihaoc@linux.ibm.com>

gcc/
	* config/rs6000/predicates.md (u2bit_cint_operand): Defined.
	* 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.
	* config/rs6000/rs6000.md (UNSPECV_MFFSCRNI): Defined.
	(rs6000_mffscrni): Defined.
	(rs6000_set_fpscr_rn): Change the type of operand[0] form DI to SI.
	Call gen_rs6000_mffscrni when operand[0] is a const int[0,3].

gcc/testsuite/
	* gcc.target/powerpc/mffscrni_p9.c: New testcase for mffscrni.
	* gcc.target/powerpc/test_fpscr_rn_builtin.c: Modify the test cases to
	test mffscrn and mffscrni separately.


patch.diff
  

On Mon, Dec 20, 2021 at 12:56 AM HAO CHEN GUI <guihaoc@linux.ibm.com> wrote:
>
> Hi,
>   I modified the patch according to David and Segher's advice.
>
>   This patch defines a pattern for mffscrni. If the RN is a constant, it can call
> gen_rs6000_mffscrni directly. The "rs6000-builtin-new.def" defines prototype for builtin arguments.
> The pattern "rs6000_set_fpscr_rn" is then broken as the mode of its argument is DI while its
> corresponding builtin has a const int argument. The patch also fixed it.
>
>   Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. Is this okay for trunk?
> Any recommendations? Thanks a lot.
>
> ChangeLog
> 2021-12-17 Haochen Gui <guihaoc@linux.ibm.com>
>
> gcc/
>         * config/rs6000/predicates.md (u2bit_cint_operand): Defined.
>         * 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.
>         * config/rs6000/rs6000.md (UNSPECV_MFFSCRNI): Defined.
>         (rs6000_mffscrni): Defined.
>         (rs6000_set_fpscr_rn): Change the type of operand[0] form DI to SI.
>         Call gen_rs6000_mffscrni when operand[0] is a const int[0,3].
>
> gcc/testsuite/
>         * gcc.target/powerpc/mffscrni_p9.c: New testcase for mffscrni.
>         * gcc.target/powerpc/test_fpscr_rn_builtin.c: Modify the test cases to
>         test mffscrn and mffscrni separately.

This revised patch is okay.

Thanks, David

>
>
> patch.diff
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index f216ffdf410..b10b4ce6065 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -219,6 +219,11 @@ (define_predicate "u1bit_cint_operand"
>    (and (match_code "const_int")
>         (match_test "INTVAL (op) >= 0 && INTVAL (op) <= 1")))
>
> +;; 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")))
> +
>  ;; Return 1 if op is a unsigned 3-bit constant integer.
>  (define_predicate "u3bit_cint_operand"
>    (and (match_code "const_int")
> diff --git a/gcc/config/rs6000/rs6000-call.c b/gcc/config/rs6000/rs6000-call.c
> index d9736eaf21c..81261a0f24d 100644
> --- a/gcc/config/rs6000/rs6000-call.c
> +++ b/gcc/config/rs6000/rs6000-call.c
> @@ -9610,13 +9610,15 @@ rs6000_expand_set_fpscr_rn_builtin (enum insn_code icode, tree exp)
>       compile time if the argument is a variable.  The least significant two
>       bits of the argument, regardless of type, are used to set the rounding
>       mode.  All other bits are ignored.  */
> -  if (CONST_INT_P (op0) && !const_0_to_3_operand(op0, VOIDmode))
> +  if (CONST_INT_P (op0))
>      {
> -      error ("Argument must be a value between 0 and 3.");
> -      return const0_rtx;
> +      if (!const_0_to_3_operand (op0, VOIDmode))
> +       {
> +         error ("Argument must be a value between 0 and 3.");
> +         return const0_rtx;
> +       }
>      }
> -
> -  if (! (*insn_data[icode].operand[0].predicate) (op0, mode0))
> +  else if (! (*insn_data[icode].operand[0].predicate) (op0, mode0))
>      op0 = copy_to_mode_reg (mode0, op0);
>
>    pat = GEN_FCN (icode) (op0);
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 6bec2bddbde..b18746af7ea 100644
> --- 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
>     UNSPECV_MFFSCDRN            ; Move from FPSCR decimal float rounding mode
>     UNSPECV_MTFSF               ; Move to FPSCR Fields 8 to 15
>     UNSPECV_MTFSF_HI            ; Move to FPSCR Fields 0 to 7
> @@ -6315,6 +6316,14 @@ (define_insn "rs6000_mffscrn"
>     "mffscrn %0,%1"
>    [(set_attr "type" "fp")])
>
> +(define_insn "rs6000_mffscrni"
> +  [(set (match_operand:DF 0 "gpc_reg_operand" "=d")
> +       (unspec_volatile:DF [(match_operand:SI 1 "u2bit_cint_operand" "n")]
> +                           UNSPECV_MFFSCRNI))]
> +   "TARGET_P9_MISC"
> +   "mffscrni %0,%1"
> +  [(set_attr "type" "fp")])
> +
>  (define_insn "rs6000_mffscdrn"
>    [(set (match_operand:DF 0 "gpc_reg_operand" "=d")
>     (unspec_volatile:DF [(const_int 0)] UNSPECV_MFFSCDRN))
> @@ -6324,7 +6333,7 @@ (define_insn "rs6000_mffscdrn"
>    [(set_attr "type" "fp")])
>
>  (define_expand "rs6000_set_fpscr_rn"
> - [(match_operand:DI 0 "reg_or_cint_operand")]
> + [(match_operand:SI 0 "reg_or_cint_operand")]
>    "TARGET_HARD_FLOAT"
>  {
>    rtx tmp_df = gen_reg_rtx (DFmode);
> @@ -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))
> +       emit_insn (gen_rs6000_mffscrni (tmp_df, operands[0]));
> +      else
> +       {
> +         rtx op0 = convert_to_mode (DImode, operands[0], false);
> +         rtx src_df = simplify_gen_subreg (DFmode, op0, DImode, 0);
> +         emit_insn (gen_rs6000_mffscrn (tmp_df, src_df));
> +       }
>        DONE;
>      }
>
> @@ -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)));
>
>        /* Insert new RN mode into FSCPR.  */
>        emit_insn (gen_rs6000_mffs (tmp_df));
> diff --git a/gcc/testsuite/gcc.target/powerpc/mffscrni_p9.c b/gcc/testsuite/gcc.target/powerpc/mffscrni_p9.c
> new file mode 100644
> index 00000000000..989a5bcc949
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/mffscrni_p9.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile { target { has_arch_pwr9 } } } */
> +/* { dg-options "-O2" } */
> +/* { dg-final { scan-assembler-times {\mmffscrni\M} 1 } } */
> +
> +void foo ()
> +{
> +  int val = 2;
> +  __builtin_set_fpscr_rn (val);
> +}
> diff --git a/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> index 0d0d3f0f96b..594b458f62c 100644
> --- a/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> +++ b/gcc/testsuite/gcc.target/powerpc/test_fpscr_rn_builtin.c
> @@ -8,6 +8,10 @@
>  #define RN_MASK  0x3LL             /* RN field mask */
>
>  void abort (void);
> +void __attribute__ ((noinline)) wrap_set_fpscr_rn (int val)
> +{
> +  __builtin_set_fpscr_rn (val);
> +}
>
>  int main ()
>  {
> @@ -43,7 +47,8 @@ int main ()
>      }
>
>    /* Test float rounding mode builtin with const value argument.  */
> -  __builtin_set_fpscr_rn(3);
> +  val = 3;
> +  __builtin_set_fpscr_rn (val);
>    conv_val.d = __builtin_mffs();
>    ll_value = conv_val.ll & RN_MASK;
>
> @@ -58,7 +63,7 @@ int main ()
>      }
>
>    val = 2;
> -  __builtin_set_fpscr_rn(val);
> +  __builtin_set_fpscr_rn (val);
>    conv_val.d = __builtin_mffs();
>    ll_value = conv_val.ll & RN_MASK;
>
> @@ -74,7 +79,7 @@ int main ()
>
>    /* Reset to 0 for testing */
>    val = 0;
> -  __builtin_set_fpscr_rn(val);
> +  __builtin_set_fpscr_rn (val);
>
>    __builtin_mtfsb1(31);
>    conv_val.d = __builtin_mffs();
> @@ -157,7 +162,7 @@ int main ()
>
>    /* Test builtin float rounding mode with variable as argument.  */
>    val = 0;
> -  __builtin_set_fpscr_rn(val);
> +  wrap_set_fpscr_rn (val);
>    conv_val.d = __builtin_mffs();
>    ll_value = conv_val.ll & RN_MASK;
>
> @@ -172,7 +177,7 @@ int main ()
>      }
>
>    val = 3;
> -  __builtin_set_fpscr_rn(val);
> +  wrap_set_fpscr_rn (val);
>    conv_val.d = __builtin_mffs();
>    ll_value = conv_val.ll & RN_MASK;
>
  

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