[PATCH] expmed: TRUNCATE value1 if needed in store_bit_field_using_insv

[YunQiang Su]

Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
>
> YunQiang Su <syq@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" } } */