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

[Kito Cheng]

Hi Philipp:

I would suggest add define_expand pattern for bswaphi2 rather than
changing expand_unop with following reasons:

- There is a comment above this change, and it also tried widen_bswap
after this if-block,
  so I think this patch is kind of violating this comment.
     /* HImode is special because in this mode BSWAP is equivalent to ROTATE
        or ROTATERT.  First try these directly; if this fails, then try the
        obvious pair of shifts with allowed widening, as this will probably
        be always more efficient than the other fallback methods.  */

- This change doesn't improve the code gen without bswapsi2 or bswapdi2,
  (e.g. rv64gc result same code) and this also might also affect other targets,
  but we didn't have evidence it will always get better results, so I guess at
  least we should add a target hook for this.

- ...I didn't have permission to approve this change since it's not
part of RISC-V back-end :p

On Thu, Nov 11, 2021 at 10:10 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;
> +
>           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
>