Re: [PATCH v3, rs6000] Implement mffscrni pattern
Hi Segher,
Thanks for your advice. Please see my explanation below.
On 22/12/2021 上午 1:05, Segher Boessenkool wrote:
> Hi!
>
> On Tue, Dec 21, 2021 at 04:08:06PM +0800, HAO CHEN GUI wrote:
>> This patch defines a pattern for mffscrni. If the RN is a constant, it can
>> call
>> gen_rs6000_mffscrni directly.
>
> And that isn't more work than just falling through to the general case.
> Okay.
>
>> 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.
>
> I don't understand that bit. Do you mean it is a const_int, or do you
> mean it is an "int" as in C source code, i.e. a 32-bit integer?
With the trunk, the new test case hits following ICE.
test_fpscr_rn_builtin.c: In function ‘wrap_set_fpscr_rn’:
test_fpscr_rn_builtin.c:13:3: internal compiler error: in copy_to_mode_reg, at
explow.c:652
13 | __builtin_set_fpscr_rn (val);
| ^~~~
0x1065892f copy_to_mode_reg(machine_mode, rtx_def*)
/home/guihaoc/gcc/gcc-mainline-base/gcc/explow.c:652
0x11206127 rs6000_expand_new_builtin
/home/guihaoc/gcc/gcc-mainline-base/gcc/config/rs6000/rs6000-call.c:15974
0x11206127 rs6000_expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode,
int)
/home/guihaoc/gcc/gcc-mainline-base/gcc/config/rs6000/rs6000-call.c:14591
The val is int variable, so its mode is SImode. But rs6000_set_fpscr_rn
requires DImode.
for (int i = 0; i < nargs; i++)
if (!insn_data[icode].operand[i+k].predicate (op[i], mode[i+k]))
op[i] = copy_to_mode_reg (mode[i+k], op[i]);
So it executes copy_to_mode_reg to copy it to DImode. Then it fails at
assertion in
copy_to_mode_reg.
gcc_assert (GET_MODE (x) == mode || GET_MODE (x) == VOIDmode);
The original test case only tests the val as const int(E_VOIDmode). So it never
hits
copy_to_mode_reg.
>
>> * 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.
>
> "Do not copy"
>
>> * config/rs6000/rs6000.md (rs6000_mffscrni): Defined.
>
> "Define".
>
>> (rs6000_set_fpscr_rn): Change the type of operand[0] form DI to SI.
>
> "from"
>
>> Call gen_rs6000_mffscrni when operand[0] is a const int[0,3].
>
> "if operands[0] is a const_0_to_3_operand"
>
>> * 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.
>
> "Test for mffscrn and mffscrni separately."
>
> Everything you say in a changelog is "modify this" or "modify that", and
> (almost) all things in gcc/testsuite/ are testcases :-)
>
>> @@ -6357,7 +6370,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 (3)));
>>
>>/* Insert new RN mode into FSCPR. */
>>emit_insn (gen_rs6000_mffs (tmp_df));
>
> It doesn't seem correct to use DImode with -m32, hrm. Not new of
> course, but I wonder how this worked.
With m32 on BE, it sets the low part to 0 when converting SI to DI.
As it just needs last two bits of high part, I set the signed to false.
rtx op0 = convert_to_mode (DImode, operands[0], false)
(insn 110 109 111 21 (set (subreg:SI (reg:DI 179 [ ll_value ]) 0)
(const_int 0 [0])) "test_fpscr_rn_builtin.c":167:12 556
{*movsi_internal1}
(nil))
>
> Okay for trunk with such changelog fixes. Thanks!
>
>
> Segher
Re: [PATCH v3, rs6000] Implement mffscrni pattern
Hi! On Tue, Dec 21, 2021 at 04:08:06PM +0800, HAO CHEN GUI wrote: > This patch defines a pattern for mffscrni. If the RN is a constant, it can > call > gen_rs6000_mffscrni directly. And that isn't more work than just falling through to the general case. Okay. > 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. I don't understand that bit. Do you mean it is a const_int, or do you mean it is an "int" as in C source code, i.e. a 32-bit integer? > * 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. "Do not copy" > * config/rs6000/rs6000.md (rs6000_mffscrni): Defined. "Define". > (rs6000_set_fpscr_rn): Change the type of operand[0] form DI to SI. "from" > Call gen_rs6000_mffscrni when operand[0] is a const int[0,3]. "if operands[0] is a const_0_to_3_operand" > * 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. "Test for mffscrn and mffscrni separately." Everything you say in a changelog is "modify this" or "modify that", and (almost) all things in gcc/testsuite/ are testcases :-) > @@ -6357,7 +6370,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 (3))); > >/* Insert new RN mode into FSCPR. */ >emit_insn (gen_rs6000_mffs (tmp_df)); It doesn't seem correct to use DImode with -m32, hrm. Not new of course, but I wonder how this worked. Okay for trunk with such changelog fixes. Thanks! Segher
[PATCH v3, rs6000] Implement mffscrni pattern
Hi,
I modified the patch according to reviewers' 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-21 Haochen Gui
gcc/
* 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 (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
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..452a77f2033 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -6315,6 +6315,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 "const_0_to_3_operand" "n")]
+ UNSPECV_MFFSCRN))]
+ "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 +6332,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 +6341,14 @@ (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_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 +6370,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 (3)));
/* 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 000..d97c6db8002
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/mffscrni_p9.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mdejagnu-cpu=power9" } */
+/* { 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..04707ad8a56
