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.
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.
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.
> 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));
> }
>
> (also inclues unnesting of the else). Could you try changing the code
> to do that and push the change if it works?
>
> IMO the patch (in that form) is OK for backports after it has had a
> couple of weeks on trunk.
>
> Thanks,
> Richard
>
> > diff --git a/gcc/testsuite/gcc.target/mips/pr113179.c
> > b/gcc/testsuite/gcc.target/mips/pr113179.c
> > new file mode 100644
> > index 00000000000..f32c5a16765
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/mips/pr113179.c
> > @@ -0,0 +1,18 @@
> > +/* Check if the operand of INS is sign-extended on MIPS64. */
> > +/* { dg-options "-mips64r2 -mabi=64" } */
> > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> > +
> > +struct xx {
> > + int a:1;
> > + int b:24;
> > + int c:6;
> > + int d:1;
> > +};
> > +
> > +long long xx (struct xx *a, long long b) {
> > + a->d = b;
> > + return b+1;
> > +}
> > +
> > +/* { dg-final { scan-assembler "\tsll\t\\\$3,\\\$5,0" } } */
> > +/* { dg-final { scan-assembler "\tdaddiu\t\\\$2,\\\$5,1" } } */
