Re: [PATCH v1 1/8] bswap: synthesize HImode bswap from SImode or DImode

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Nov 19, 2021 at 11:20 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 3:13 PM Philipp Tomsich
> <philipp.tomsich@vrull.eu> wrote:
> >
> > The RISC-V Zbb extension adds an XLEN (i.e. SImode for rv32, DImode
> > for rv64) bswap instruction (rev8).  While, with the current master,
> > SImode is synthesized correctly from DImode, HImode is not.
> >
> > This change adds an appropriate expansion for a HImode bswap, if a
> > wider bswap is available.
> >
> > Without this change, the following rv64gc_zbb code is generated for
> > __builtin_bswap16():
> >         slliw   a5,a0,8
> >         zext.h  a0,a0
> >         srliw   a0,a0,8
> >         or      a0,a5,a0
> >         sext.h  a0,a0      // this is a 16bit sign-extension following
> >                            // the byteswap (e.g. on a 'short' function
> >                            // return).
> >
> > After this change, a bswap (rev8) is used and any extensions are
> > combined into the shift-right:
> >         rev8    a0,a0
> >         srai    a0,a0,48   // the sign-extension is combined into the
> >                            // shift; a srli is emitted otherwise...
> >
> > gcc/ChangeLog:
> >
> >         * optabs.c (expand_unop): support expanding a HImode bswap
> >           using SImode or DImode, followed by a shift.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/riscv/zbb-bswap.c: New test.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >  gcc/optabs.c                               |  6 ++++++
> >  gcc/testsuite/gcc.target/riscv/zbb-bswap.c | 22 ++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-bswap.c
> >
> > diff --git a/gcc/optabs.c b/gcc/optabs.c
> > index 019bbb62882..7a3ffbe4525 100644
> > --- a/gcc/optabs.c
> > +++ b/gcc/optabs.c
> > @@ -3307,6 +3307,12 @@ expand_unop (machine_mode mode, optab unoptab, rtx op0, rtx target,
> >                 return temp;
> >             }
> >
> > +         /* If we are missing a HImode BSWAP, but have one for SImode or
> > +            DImode, use a BSWAP followed by a SHIFT.  */
> > +         temp = widen_bswap (as_a <scalar_int_mode> (mode), op0, target);
> > +         if (temp)
> > +           return temp;
> > +
>
> I think it would be more natural to temporarily terminate the HImode case here
> and re-open it inside the following
>
>       if (is_a <scalar_int_mode> (mode, &int_mode))
>         {
>           temp = widen_bswap (int_mode, op0, target);
>           if (temp)
>             return temp;
>
> here to handle the ashl/lshr/ior fallback.

But as Kito says, code generation for more targets would need to be looked at.

Richard.

> Richard.
>
> >           last = get_last_insn ();
> >
> >           temp1 = expand_binop (mode, ashl_optab, op0,
> > diff --git a/gcc/testsuite/gcc.target/riscv/zbb-bswap.c b/gcc/testsuite/gcc.target/riscv/zbb-bswap.c
> > new file mode 100644
> > index 00000000000..6ee27d9f47a
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/zbb-bswap.c
> > @@ -0,0 +1,22 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc_zbb -mabi=lp64 -O2" } */
> > +
> > +unsigned long
> > +func64 (unsigned long i)
> > +{
> > +  return __builtin_bswap64(i);
> > +}
> > +
> > +unsigned int
> > +func32 (unsigned int i)
> > +{
> > +  return __builtin_bswap32(i);
> > +}
> > +
> > +unsigned short
> > +func16 (unsigned short i)
> > +{
> > +  return __builtin_bswap16(i);
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "rev8" 3 } } */
> > --
> > 2.32.0
> >