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

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

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.

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
>