YunQiang Su <s...@gcc.gnu.org> writes:
> Richard Sandiford <richard.sandif...@arm.com> 于2024年6月5日周三 23:20写道：
>>
>> 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.
>>
>
> Thanks for your explanation. NVPTX defines TRULY_NOOP_TRUNCATION_MODES_P
> to always false.
Yeah. AIUI, the change above behaves correcty there. It maintains
the type correctness of the output.
> and
>
> bool
> gcn_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec)
> {
> return ((inprec <= 32) && (outprec <= inprec));
> }
Not sure why GCN requires this TBH, but...
> I am worrying that we may break nvptx/amdgcn as I have little
> knowledge about them.
...I think the targets are asking for the same behaviour as MIPS.
The reason for the patch is that the code really is doing a
truncation from value_mode to op_mode. If the target says that
that isn't a no-op, we have to use the target's truncation pattern
instead of a subreg.
Thanks,
Richard
>
>
>> > 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
