RISC-V: Disable Zba optimization pattern if XTheadMemIdx is enabled

It is possible that the Zba optimization pattern zero_extendsidi2_bitmanip
matches for a XTheadMemIdx INSN with the effect of emitting an invalid
instruction as reported in PR116035.

The pattern above is used to emit a zext.w instruction to zero-extend
SI mode registers to DI mode.  A similar functionality can be achieved
by XTheadBb's th.extu instruction.  And indeed, we have the equivalent
pattern in thead.md (zero_extendsidi2_th_extu).  However, that pattern
depends on !TARGET_XTHEADMEMIDX.  To compensate for that, there are
specific patterns that ensure that zero-extension instruction can still
be emitted (th_memidx_bb_zero_extendsidi2 and friends).

While we could implement something similar (th_memidx_zba_zero_extendsidi2)
it would only make sense, if there existed real HW that does implement Zba
and XTheadMemIdx, but not XTheadBb.  Unless such a machine exists, let's
simply disable zero_extendsidi2_bitmanip if XTheadMemIdx is available.

	PR target/116035

gcc/ChangeLog:

	* config/riscv/bitmanip.md: Disable zero_extendsidi2_bitmanip
	for XTheadMemIdx.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/pr116035-1.c: New test.
	* gcc.target/riscv/pr116035-2.c: New test.

Reported-by: Patrick O'Neill <patrick@rivosinc.com>
Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 gcc/config/riscv/bitmanip.md                |  2 +-
 gcc/testsuite/gcc.target/riscv/pr116035-1.c | 29 +++++++++++++++++++++
 gcc/testsuite/gcc.target/riscv/pr116035-2.c | 26 ++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-2.c

LGTM :)

On Wed, Jul 24, 2024 at 3:16 PM Christoph Müllner
<christoph.muellner@vrull.eu> wrote:
>
> It is possible that the Zba optimization pattern zero_extendsidi2_bitmanip
> matches for a XTheadMemIdx INSN with the effect of emitting an invalid
> instruction as reported in PR116035.
>
> The pattern above is used to emit a zext.w instruction to zero-extend
> SI mode registers to DI mode.  A similar functionality can be achieved
> by XTheadBb's th.extu instruction.  And indeed, we have the equivalent
> pattern in thead.md (zero_extendsidi2_th_extu).  However, that pattern
> depends on !TARGET_XTHEADMEMIDX.  To compensate for that, there are
> specific patterns that ensure that zero-extension instruction can still
> be emitted (th_memidx_bb_zero_extendsidi2 and friends).
>
> While we could implement something similar (th_memidx_zba_zero_extendsidi2)
> it would only make sense, if there existed real HW that does implement Zba
> and XTheadMemIdx, but not XTheadBb.  Unless such a machine exists, let's
> simply disable zero_extendsidi2_bitmanip if XTheadMemIdx is available.
>
>         PR target/116035
>
> gcc/ChangeLog:
>
>         * config/riscv/bitmanip.md: Disable zero_extendsidi2_bitmanip
>         for XTheadMemIdx.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/pr116035-1.c: New test.
>         * gcc.target/riscv/pr116035-2.c: New test.
>
> Reported-by: Patrick O'Neill <patrick@rivosinc.com>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>  gcc/config/riscv/bitmanip.md                |  2 +-
>  gcc/testsuite/gcc.target/riscv/pr116035-1.c | 29 +++++++++++++++++++++
>  gcc/testsuite/gcc.target/riscv/pr116035-2.c | 26 ++++++++++++++++++
>  3 files changed, 56 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-1.c
>  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-2.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index f403ba8dbba..6b720992ca3 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -22,7 +22,7 @@
>  (define_insn "*zero_extendsidi2_bitmanip"
>    [(set (match_operand:DI 0 "register_operand" "=r,r")
>         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "r,m")))]
> -  "TARGET_64BIT && TARGET_ZBA"
> +  "TARGET_64BIT && TARGET_ZBA && !TARGET_XTHEADMEMIDX"
>    "@
>     zext.w\t%0,%1
>     lwu\t%0,%1"
> diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-1.c b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> new file mode 100644
> index 00000000000..bc45941ff8f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> +/* { dg-options "-march=rv64g_zba_xtheadmemidx" { target { rv64 } } } */
> +/* { dg-options "-march=rv32g_zba_xtheadmemidx" { target { rv32 } } } */
> +
> +void a(long);
> +unsigned b[11];
> +void c()
> +{
> +  for (int d = 0; d < 11; ++d)
> +    a(b[d]);
> +}
> +
> +#if __riscv_xlen == 64
> +unsigned long zext64_32(unsigned int u32)
> +{
> +  /* Missed optimization for Zba+XTheadMemIdx.  */
> +  return u32; //zext.w a0, a0
> +}
> +#endif
> +
> +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv64 } } } */
> +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv32 } } } */
> +
> +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\(\[a-x0-9\]+\),4,0" } } */
> +
> +/* Missed optimizations for Zba+XTheadMemIdx.  */
> +/* { dg-final { scan-assembler "zext.w\t" { target rv64 xfail rv64 } } } */
> +
> diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-2.c b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> new file mode 100644
> index 00000000000..2c1a9694860
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> @@ -0,0 +1,26 @@
> +/* { dg-do compile } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> +/* { dg-options "-march=rv64g_xtheadbb_xtheadmemidx" { target { rv64 } } } */
> +/* { dg-options "-march=rv32g_xtheadbb_xtheadmemidx" { target { rv32 } } } */
> +
> +void a(long);
> +unsigned b[11];
> +void c()
> +{
> +  for (int d = 0; d < 11; ++d)
> +    a(b[d]);
> +}
> +
> +#if __riscv_xlen == 64
> +unsigned long zext64_32(unsigned int u32)
> +{
> +    return u32; //th.extu a0, a0, 31, 0
> +}
> +#endif
> +
> +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv64 } } } } */
> +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv32 } } } } */
> +
> +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" } } */
> +
> +/* { dg-final { scan-assembler "th.extu\t" { target rv64 } } } */
> --
> 2.45.2
>

Is it OK to backport to GCC 14 (patch applies cleanly, test is running)?

On Wed, Jul 24, 2024 at 9:25 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>
> LGTM :)
>
> On Wed, Jul 24, 2024 at 3:16 PM Christoph Müllner
> <christoph.muellner@vrull.eu> wrote:
> >
> > It is possible that the Zba optimization pattern zero_extendsidi2_bitmanip
> > matches for a XTheadMemIdx INSN with the effect of emitting an invalid
> > instruction as reported in PR116035.
> >
> > The pattern above is used to emit a zext.w instruction to zero-extend
> > SI mode registers to DI mode.  A similar functionality can be achieved
> > by XTheadBb's th.extu instruction.  And indeed, we have the equivalent
> > pattern in thead.md (zero_extendsidi2_th_extu).  However, that pattern
> > depends on !TARGET_XTHEADMEMIDX.  To compensate for that, there are
> > specific patterns that ensure that zero-extension instruction can still
> > be emitted (th_memidx_bb_zero_extendsidi2 and friends).
> >
> > While we could implement something similar (th_memidx_zba_zero_extendsidi2)
> > it would only make sense, if there existed real HW that does implement Zba
> > and XTheadMemIdx, but not XTheadBb.  Unless such a machine exists, let's
> > simply disable zero_extendsidi2_bitmanip if XTheadMemIdx is available.
> >
> >         PR target/116035
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/bitmanip.md: Disable zero_extendsidi2_bitmanip
> >         for XTheadMemIdx.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/riscv/pr116035-1.c: New test.
> >         * gcc.target/riscv/pr116035-2.c: New test.
> >
> > Reported-by: Patrick O'Neill <patrick@rivosinc.com>
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >  gcc/config/riscv/bitmanip.md                |  2 +-
> >  gcc/testsuite/gcc.target/riscv/pr116035-1.c | 29 +++++++++++++++++++++
> >  gcc/testsuite/gcc.target/riscv/pr116035-2.c | 26 ++++++++++++++++++
> >  3 files changed, 56 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-1.c
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-2.c
> >
> > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > index f403ba8dbba..6b720992ca3 100644
> > --- a/gcc/config/riscv/bitmanip.md
> > +++ b/gcc/config/riscv/bitmanip.md
> > @@ -22,7 +22,7 @@
> >  (define_insn "*zero_extendsidi2_bitmanip"
> >    [(set (match_operand:DI 0 "register_operand" "=r,r")
> >         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "r,m")))]
> > -  "TARGET_64BIT && TARGET_ZBA"
> > +  "TARGET_64BIT && TARGET_ZBA && !TARGET_XTHEADMEMIDX"
> >    "@
> >     zext.w\t%0,%1
> >     lwu\t%0,%1"
> > diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-1.c b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> > new file mode 100644
> > index 00000000000..bc45941ff8f
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> > @@ -0,0 +1,29 @@
> > +/* { dg-do compile } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> > +/* { dg-options "-march=rv64g_zba_xtheadmemidx" { target { rv64 } } } */
> > +/* { dg-options "-march=rv32g_zba_xtheadmemidx" { target { rv32 } } } */
> > +
> > +void a(long);
> > +unsigned b[11];
> > +void c()
> > +{
> > +  for (int d = 0; d < 11; ++d)
> > +    a(b[d]);
> > +}
> > +
> > +#if __riscv_xlen == 64
> > +unsigned long zext64_32(unsigned int u32)
> > +{
> > +  /* Missed optimization for Zba+XTheadMemIdx.  */
> > +  return u32; //zext.w a0, a0
> > +}
> > +#endif
> > +
> > +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv64 } } } */
> > +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv32 } } } */
> > +
> > +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\(\[a-x0-9\]+\),4,0" } } */
> > +
> > +/* Missed optimizations for Zba+XTheadMemIdx.  */
> > +/* { dg-final { scan-assembler "zext.w\t" { target rv64 xfail rv64 } } } */
> > +
> > diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-2.c b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> > new file mode 100644
> > index 00000000000..2c1a9694860
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> > @@ -0,0 +1,26 @@
> > +/* { dg-do compile } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> > +/* { dg-options "-march=rv64g_xtheadbb_xtheadmemidx" { target { rv64 } } } */
> > +/* { dg-options "-march=rv32g_xtheadbb_xtheadmemidx" { target { rv32 } } } */
> > +
> > +void a(long);
> > +unsigned b[11];
> > +void c()
> > +{
> > +  for (int d = 0; d < 11; ++d)
> > +    a(b[d]);
> > +}
> > +
> > +#if __riscv_xlen == 64
> > +unsigned long zext64_32(unsigned int u32)
> > +{
> > +    return u32; //th.extu a0, a0, 31, 0
> > +}
> > +#endif
> > +
> > +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv64 } } } } */
> > +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv32 } } } } */
> > +
> > +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" } } */
> > +
> > +/* { dg-final { scan-assembler "th.extu\t" { target rv64 } } } */
> > --
> > 2.45.2
> >

Yeah, OK once your local test passes :)

On Wed, Jul 24, 2024 at 4:38 PM Christoph Müllner
<christoph.muellner@vrull.eu> wrote:
>
> Is it OK to backport to GCC 14 (patch applies cleanly, test is running)?
>
> On Wed, Jul 24, 2024 at 9:25 AM Kito Cheng <kito.cheng@sifive.com> wrote:
> >
> > LGTM :)
> >
> > On Wed, Jul 24, 2024 at 3:16 PM Christoph Müllner
> > <christoph.muellner@vrull.eu> wrote:
> > >
> > > It is possible that the Zba optimization pattern zero_extendsidi2_bitmanip
> > > matches for a XTheadMemIdx INSN with the effect of emitting an invalid
> > > instruction as reported in PR116035.
> > >
> > > The pattern above is used to emit a zext.w instruction to zero-extend
> > > SI mode registers to DI mode.  A similar functionality can be achieved
> > > by XTheadBb's th.extu instruction.  And indeed, we have the equivalent
> > > pattern in thead.md (zero_extendsidi2_th_extu).  However, that pattern
> > > depends on !TARGET_XTHEADMEMIDX.  To compensate for that, there are
> > > specific patterns that ensure that zero-extension instruction can still
> > > be emitted (th_memidx_bb_zero_extendsidi2 and friends).
> > >
> > > While we could implement something similar (th_memidx_zba_zero_extendsidi2)
> > > it would only make sense, if there existed real HW that does implement Zba
> > > and XTheadMemIdx, but not XTheadBb.  Unless such a machine exists, let's
> > > simply disable zero_extendsidi2_bitmanip if XTheadMemIdx is available.
> > >
> > >         PR target/116035
> > >
> > > gcc/ChangeLog:
> > >
> > >         * config/riscv/bitmanip.md: Disable zero_extendsidi2_bitmanip
> > >         for XTheadMemIdx.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/riscv/pr116035-1.c: New test.
> > >         * gcc.target/riscv/pr116035-2.c: New test.
> > >
> > > Reported-by: Patrick O'Neill <patrick@rivosinc.com>
> > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > > ---
> > >  gcc/config/riscv/bitmanip.md                |  2 +-
> > >  gcc/testsuite/gcc.target/riscv/pr116035-1.c | 29 +++++++++++++++++++++
> > >  gcc/testsuite/gcc.target/riscv/pr116035-2.c | 26 ++++++++++++++++++
> > >  3 files changed, 56 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-1.c
> > >  create mode 100644 gcc/testsuite/gcc.target/riscv/pr116035-2.c
> > >
> > > diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> > > index f403ba8dbba..6b720992ca3 100644
> > > --- a/gcc/config/riscv/bitmanip.md
> > > +++ b/gcc/config/riscv/bitmanip.md
> > > @@ -22,7 +22,7 @@
> > >  (define_insn "*zero_extendsidi2_bitmanip"
> > >    [(set (match_operand:DI 0 "register_operand" "=r,r")
> > >         (zero_extend:DI (match_operand:SI 1 "nonimmediate_operand" "r,m")))]
> > > -  "TARGET_64BIT && TARGET_ZBA"
> > > +  "TARGET_64BIT && TARGET_ZBA && !TARGET_XTHEADMEMIDX"
> > >    "@
> > >     zext.w\t%0,%1
> > >     lwu\t%0,%1"
> > > diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-1.c b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> > > new file mode 100644
> > > index 00000000000..bc45941ff8f
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/pr116035-1.c
> > > @@ -0,0 +1,29 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> > > +/* { dg-options "-march=rv64g_zba_xtheadmemidx" { target { rv64 } } } */
> > > +/* { dg-options "-march=rv32g_zba_xtheadmemidx" { target { rv32 } } } */
> > > +
> > > +void a(long);
> > > +unsigned b[11];
> > > +void c()
> > > +{
> > > +  for (int d = 0; d < 11; ++d)
> > > +    a(b[d]);
> > > +}
> > > +
> > > +#if __riscv_xlen == 64
> > > +unsigned long zext64_32(unsigned int u32)
> > > +{
> > > +  /* Missed optimization for Zba+XTheadMemIdx.  */
> > > +  return u32; //zext.w a0, a0
> > > +}
> > > +#endif
> > > +
> > > +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv64 } } } */
> > > +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target rv32 } } } */
> > > +
> > > +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\(\[a-x0-9\]+\),4,0" } } */
> > > +
> > > +/* Missed optimizations for Zba+XTheadMemIdx.  */
> > > +/* { dg-final { scan-assembler "zext.w\t" { target rv64 xfail rv64 } } } */
> > > +
> > > diff --git a/gcc/testsuite/gcc.target/riscv/pr116035-2.c b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> > > new file mode 100644
> > > index 00000000000..2c1a9694860
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/pr116035-2.c
> > > @@ -0,0 +1,26 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" "-Os" "-Oz" } } */
> > > +/* { dg-options "-march=rv64g_xtheadbb_xtheadmemidx" { target { rv64 } } } */
> > > +/* { dg-options "-march=rv32g_xtheadbb_xtheadmemidx" { target { rv32 } } } */
> > > +
> > > +void a(long);
> > > +unsigned b[11];
> > > +void c()
> > > +{
> > > +  for (int d = 0; d < 11; ++d)
> > > +    a(b[d]);
> > > +}
> > > +
> > > +#if __riscv_xlen == 64
> > > +unsigned long zext64_32(unsigned int u32)
> > > +{
> > > +    return u32; //th.extu a0, a0, 31, 0
> > > +}
> > > +#endif
> > > +
> > > +/* { dg-final { scan-assembler "th.lwuia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv64 } } } } */
> > > +/* { dg-final { scan-assembler "th.lwia\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" { target { rv32 } } } } */
> > > +
> > > +/* { dg-final { scan-assembler-not "lwu\t\[a-x0-9\]+,\\(\[a-x0-9\]+\\),4,0" } } */
> > > +
> > > +/* { dg-final { scan-assembler "th.extu\t" { target rv64 } } } */
> > > --
> > > 2.45.2
> > >