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
