diff mbox series

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

Message ID 20211111141020.2738001-2-philipp.tomsich@vrull.eu
State New
Headers
Series Improvements to bitmanip-1.0 (Zb[abcs]) support |

Commit Message

Philipp Tomsich Nov. 11, 2021, 2:10 p.m. UTC 
  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

Comments

Kito Cheng Nov. 17, 2021, 2:51 p.m. UTC | #1 
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
>
Richard Biener Nov. 19, 2021, 10:20 a.m. UTC | #2 
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
>
Richard Biener Nov. 19, 2021, 10:21 a.m. UTC | #3 
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
> >
diff mbox series

Patch

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 } } */