YunQiang Su <s...@gcc.gnu.org> writes:
> Richard Sandiford <richard.sandif...@arm.com> 于2024年6月5日周三 22:14写道：
>>
>> YunQiang Su <s...@gcc.gnu.org> writes:
>> > PR target/113179.
>> >
>> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>> >>= op_mode, while in some ports, a sign_extend will be needed,
>> > such as MIPS64:
>> > If either GPR rs or GPR rt does not contain sign-extended 32-bit
>> > values (bits 63..31 equal), then the result of the operation is
>> > UNPREDICTABLE.
>> >
>> > The problem happens for the code like:
>> > struct xx {
>> > int a:4;
>> > int b:24;
>> > int c:3;
>> > int d:1;
>> > };
>> >
>> > void xx (struct xx *a, long long b) {
>> > a->d = b;
>> > }
>> >
>> > In the above code, the hard register contains `b`, may be note well
>> > sign-extended.
>> >
>> > gcc/
>> > PR target/113179
>> > * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
>> > needed.
>> >
>> > gcc/testsuite
>> > PR target/113179
>> > * gcc.target/mips/pr113179.c: New tests.
>> > ---
>> > gcc/expmed.cc | 12 +++++++++---
>> > gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>> > 2 files changed, 27 insertions(+), 3 deletions(-)
>> > create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>> >
>> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>> > index 4ec035e4843..6a582593da8 100644
>> > --- a/gcc/expmed.cc
>> > +++ b/gcc/expmed.cc
>> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn
>> > *insv, rtx op0,
>> > }
>> > else
>> > {
>> > - tmp = gen_lowpart_if_possible (op_mode, value1);
>> > - if (! tmp)
>> > - tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> > + if (targetm.mode_rep_extended (op_mode, value_mode))
>> > + tmp = simplify_gen_unary (TRUNCATE, op_mode,
>> > + value1, value_mode);
>> > + else
>> > + {
>> > + tmp = gen_lowpart_if_possible (op_mode, value1);
>> > + if (! tmp)
>> > + tmp = gen_lowpart (op_mode, force_reg (value_mode,
>> > value1));
>> > + }
>> > }
>> > value1 = tmp;
>> > }
>>
>> I notice this patch is already applied. Was it approved? I didn't
>> see an approval in my feed or in the archives.
>>
>
> Sorry. I was supposed that it only effects MIPS targets since only MIPS
> defines
> targetm.mode_rep_extended
>
>> Although it might not make any difference on current targets,
>> I think the conditional should logically be based on
>> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
>>
>> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
>> to do this truncation? targetm.mode_rep_extended is instead an
>> optimisation question: can I assume that a particular extension is free?
>> Here we're trying to avoid a subreg for correctness, rather than trying
>> to take advantage of a cheap extension.
>>
>> So I think the code should be:
>>
>> if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
>> {
>> tmp = simplify_subreg (op_mode, value1, value_mode, 0);
>> if (! tmp)
>> tmp = simplify_gen_subreg (op_mode,
>> force_reg (value_mode, value1),
>> value_mode, 0);
>> }
>> else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
>> && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
>
> In fact I don't think so. For other targets besides MIPS, it is safe even
> !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
> as INS instruction may use the low part of a register safely.
Not sure what you mean. The change above is no-op for targets other
than MIPS and SH5 (now removed). But I think it's the correct way of
representing the restriction on MIPS and SH5.
>
> It is only not true on MIPS ISA documents as
> If either GPR rs or GPR rt does not contain sign-extended 32-bit
> values (bits 63..31 equal), then the result of the operation is
> UNPREDICTABLE.
Right. The reason that TRULY_NOOP_TRUNCATION_MODES_P (SImode, DImode)
is false for MIPS isn't that a subreg is impossible on MIPS targets.
It's that the MIPS port needs to ensure that SImode values are stored
in sign-extended form in order to avoid the problem above. So a
truncation needs to be a sign-extension.
> It is very annoying. I haven't noticed a similar problem on any other
> ISA documents.
> In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
> this case.
I think the Broadcom SB-1 took advantage of the freedom.
Thanks,
Richard
