diff mbox series

[v3,04/11] riscv: thead: Add support for the XTheadBs ISA extension

Message ID 20230224055127.2500953-5-christoph.muellner@vrull.eu
State Committed
Headers
Series RISC-V: Add XThead* extension support |

Commit Message

Christoph Müllner Feb. 24, 2023, 5:51 a.m. UTC 
  From: Christoph Müllner <christoph.muellner@vrull.eu>

This patch adds support for the XTheadBs ISA extension.
The new INSN pattern is defined in a new file to separate
this vendor extension from the standard extensions.
The cost model adjustment reuses the xbs:bext cost.

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_rtx_costs): Add xthead:tst cost.
	* config/riscv/thead.md (*th_tst): New INSN.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/xtheadbs-tst.c: New test.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 gcc/config/riscv/riscv.cc                     |  4 ++--
 gcc/config/riscv/thead.md                     | 11 +++++++++++
 gcc/testsuite/gcc.target/riscv/xtheadbs-tst.c | 13 +++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadbs-tst.c

Comments

Kito Cheng Feb. 24, 2023, 7:36 a.m. UTC | #1 
> diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> index 158e9124c3a..2c684885850 100644
> --- a/gcc/config/riscv/thead.md
> +++ b/gcc/config/riscv/thead.md
> @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
>    "th.addsl\t%0,%3,%1,%2"
>    [(set_attr "type" "bitmanip")
>     (set_attr "mode" "<X:MODE>")])
> +
> +;; XTheadBs
> +
> +(define_insn "*th_tst"
> +  [(set (match_operand:X 0 "register_operand" "=r")
> +       (zero_extract:X (match_operand:X 1 "register_operand" "r")
> +                       (const_int 1)
> +                       (match_operand 2 "immediate_operand" "i")))]
> +  "TARGET_XTHEADBS"

Add range check like *bexti pattern?

TARGET_XTHEADBS && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)

> +  "th.tst\t%0,%1,%2"
> +  [(set_attr "type" "bitmanip")])
Christoph Müllner Feb. 24, 2023, 10:21 a.m. UTC | #2 
On Fri, Feb 24, 2023 at 8:37 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>
> > diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> > index 158e9124c3a..2c684885850 100644
> > --- a/gcc/config/riscv/thead.md
> > +++ b/gcc/config/riscv/thead.md
> > @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
> >    "th.addsl\t%0,%3,%1,%2"
> >    [(set_attr "type" "bitmanip")
> >     (set_attr "mode" "<X:MODE>")])
> > +
> > +;; XTheadBs
> > +
> > +(define_insn "*th_tst"
> > +  [(set (match_operand:X 0 "register_operand" "=r")
> > +       (zero_extract:X (match_operand:X 1 "register_operand" "r")
> > +                       (const_int 1)
> > +                       (match_operand 2 "immediate_operand" "i")))]
> > +  "TARGET_XTHEADBS"
>
> Add range check like *bexti pattern?
>
> TARGET_XTHEADBS && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)

Ok.

Thanks,
Christoph

>
> > +  "th.tst\t%0,%1,%2"
> > +  [(set_attr "type" "bitmanip")])
Hans-Peter Nilsson Feb. 25, 2023, 11:42 p.m. UTC | #3 
On Fri, 24 Feb 2023, Christoph Muellner wrote:
> diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> index 158e9124c3a..2c684885850 100644
> --- a/gcc/config/riscv/thead.md
> +++ b/gcc/config/riscv/thead.md
> @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
>    "th.addsl\t%0,%3,%1,%2"
>    [(set_attr "type" "bitmanip")
>     (set_attr "mode" "<X:MODE>")])
> +
> +;; XTheadBs
> +
> +(define_insn "*th_tst"
> +  [(set (match_operand:X 0 "register_operand" "=r")
> +	(zero_extract:X (match_operand:X 1 "register_operand" "r")
> +			(const_int 1)
> +			(match_operand 2 "immediate_operand" "i")))]

(Here and same elsewhere.)

You're unlikely to get other constant operands in that pattern, 
but FWIW, the actual matching pair for just CONST_INT is 
"const_int_operand" for the predicate and "n" for the 
constraint.  Using the right predicate and constraint will also 
help the generated part of recog be a few nanoseconds faster. ;)

brgds, H-P
Christoph Müllner Feb. 28, 2023, 5:49 p.m. UTC | #4 
On Sun, Feb 26, 2023 at 12:42 AM Hans-Peter Nilsson <hp@bitrange.com> wrote:
>
> On Fri, 24 Feb 2023, Christoph Muellner wrote:
> > diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> > index 158e9124c3a..2c684885850 100644
> > --- a/gcc/config/riscv/thead.md
> > +++ b/gcc/config/riscv/thead.md
> > @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
> >    "th.addsl\t%0,%3,%1,%2"
> >    [(set_attr "type" "bitmanip")
> >     (set_attr "mode" "<X:MODE>")])
> > +
> > +;; XTheadBs
> > +
> > +(define_insn "*th_tst"
> > +  [(set (match_operand:X 0 "register_operand" "=r")
> > +     (zero_extract:X (match_operand:X 1 "register_operand" "r")
> > +                     (const_int 1)
> > +                     (match_operand 2 "immediate_operand" "i")))]
>
> (Here and same elsewhere.)
>
> You're unlikely to get other constant operands in that pattern,
> but FWIW, the actual matching pair for just CONST_INT is
> "const_int_operand" for the predicate and "n" for the
> constraint.  Using the right predicate and constraint will also
> help the generated part of recog be a few nanoseconds faster. ;)

Thank you for that comment!
I think what you mean would look like this:

(define_insn "*th_tst"
  [(set (match_operand:X 0 "register_operand" "=r")
    (zero_extract:X (match_operand:X 1 "register_operand" "r")
            (match_operand 3 "const_int_operand" "n")
            (match_operand 2 "immediate_operand" "i")))]
  "TARGET_XTHEADBS && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)
   && UINTVAL (operands[3]) == 1"
  "th.tst\t%0,%1,%2"
  [(set_attr "type" "bitmanip")])

So while we have more generic form in the pattern, the condition needs
to check that the operand is equal to 1.

I can change this in the patch (I don't have strong opinions about
this and I do care about the nanosecond).
However, I think this goes beyond this patchset.
Because a single git grep shows many examples of "const_int " matches
in GCC's backends.
Examples can be found in gcc/config/riscv/bitmanip.md,
gcc/config/aarch64/aarch64.md,...
So it feels like changing the patch to use const_int_operand would go
against common practice.

@Kito: Any preferences about this?

Thanks,
Christoph
Hans-Peter Nilsson March 1, 2023, 12:19 a.m. UTC | #5 
On Tue, 28 Feb 2023, Christoph Müllner wrote:

> On Sun, Feb 26, 2023 at 12:42 AM Hans-Peter Nilsson <hp@bitrange.com> wrote:
> >
> > On Fri, 24 Feb 2023, Christoph Muellner wrote:
> > > diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> > > index 158e9124c3a..2c684885850 100644
> > > --- a/gcc/config/riscv/thead.md
> > > +++ b/gcc/config/riscv/thead.md
> > > @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
> > >    "th.addsl\t%0,%3,%1,%2"
> > >    [(set_attr "type" "bitmanip")
> > >     (set_attr "mode" "<X:MODE>")])
> > > +
> > > +;; XTheadBs
> > > +
> > > +(define_insn "*th_tst"
> > > +  [(set (match_operand:X 0 "register_operand" "=r")
> > > +     (zero_extract:X (match_operand:X 1 "register_operand" "r")
> > > +                     (const_int 1)
> > > +                     (match_operand 2 "immediate_operand" "i")))]
> >
> > (Here and same elsewhere.)
> >
> > You're unlikely to get other constant operands in that pattern,
> > but FWIW, the actual matching pair for just CONST_INT is
> > "const_int_operand" for the predicate and "n" for the
> > constraint.  Using the right predicate and constraint will also
> > help the generated part of recog be a few nanoseconds faster. ;)
> 
> Thank you for that comment!
> I think what you mean would look like this:
> 
> (define_insn "*th_tst"
>   [(set (match_operand:X 0 "register_operand" "=r")
>     (zero_extract:X (match_operand:X 1 "register_operand" "r")
>             (match_operand 3 "const_int_operand" "n")
>             (match_operand 2 "immediate_operand" "i")))]

No; misunderstanding.  Keep the (const_int 1) but replace
(match_operand 2 "immediate_operand" "i") with
(match_operand 2 "const_int_operand" "n")

brgds, H-P
Christoph Müllner March 1, 2023, 7:49 a.m. UTC | #6 
On Wed, Mar 1, 2023 at 1:19 AM Hans-Peter Nilsson <hp@bitrange.com> wrote:
>
>
>
> On Tue, 28 Feb 2023, Christoph Müllner wrote:
>
> > On Sun, Feb 26, 2023 at 12:42 AM Hans-Peter Nilsson <hp@bitrange.com> wrote:
> > >
> > > On Fri, 24 Feb 2023, Christoph Muellner wrote:
> > > > diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
> > > > index 158e9124c3a..2c684885850 100644
> > > > --- a/gcc/config/riscv/thead.md
> > > > +++ b/gcc/config/riscv/thead.md
> > > > @@ -29,3 +29,14 @@ (define_insn "*th_addsl"
> > > >    "th.addsl\t%0,%3,%1,%2"
> > > >    [(set_attr "type" "bitmanip")
> > > >     (set_attr "mode" "<X:MODE>")])
> > > > +
> > > > +;; XTheadBs
> > > > +
> > > > +(define_insn "*th_tst"
> > > > +  [(set (match_operand:X 0 "register_operand" "=r")
> > > > +     (zero_extract:X (match_operand:X 1 "register_operand" "r")
> > > > +                     (const_int 1)
> > > > +                     (match_operand 2 "immediate_operand" "i")))]
> > >
> > > (Here and same elsewhere.)
> > >
> > > You're unlikely to get other constant operands in that pattern,
> > > but FWIW, the actual matching pair for just CONST_INT is
> > > "const_int_operand" for the predicate and "n" for the
> > > constraint.  Using the right predicate and constraint will also
> > > help the generated part of recog be a few nanoseconds faster. ;)
> >
> > Thank you for that comment!
> > I think what you mean would look like this:
> >
> > (define_insn "*th_tst"
> >   [(set (match_operand:X 0 "register_operand" "=r")
> >     (zero_extract:X (match_operand:X 1 "register_operand" "r")
> >             (match_operand 3 "const_int_operand" "n")
> >             (match_operand 2 "immediate_operand" "i")))]
>
> No; misunderstanding.  Keep the (const_int 1) but replace
> (match_operand 2 "immediate_operand" "i") with
> (match_operand 2 "const_int_operand" "n")

Ah, yes, this makes sense!

Thanks!


>
> brgds, H-P
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index f11b7949a49..e35bc0a745b 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2400,8 +2400,8 @@  riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
 	  *total = COSTS_N_INSNS (SINGLE_SHIFT_COST);
 	  return true;
 	}
-      /* bext pattern for zbs.  */
-      if (TARGET_ZBS && outer_code == SET
+      /* bit extraction pattern (zbs:bext, xtheadbs:tst).  */
+      if ((TARGET_ZBS || TARGET_XTHEADBS) && outer_code == SET
 	  && GET_CODE (XEXP (x, 1)) == CONST_INT
 	  && INTVAL (XEXP (x, 1)) == 1)
 	{
diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
index 158e9124c3a..2c684885850 100644
--- a/gcc/config/riscv/thead.md
+++ b/gcc/config/riscv/thead.md
@@ -29,3 +29,14 @@  (define_insn "*th_addsl"
   "th.addsl\t%0,%3,%1,%2"
   [(set_attr "type" "bitmanip")
    (set_attr "mode" "<X:MODE>")])
+
+;; XTheadBs
+
+(define_insn "*th_tst"
+  [(set (match_operand:X 0 "register_operand" "=r")
+	(zero_extract:X (match_operand:X 1 "register_operand" "r")
+			(const_int 1)
+			(match_operand 2 "immediate_operand" "i")))]
+  "TARGET_XTHEADBS"
+  "th.tst\t%0,%1,%2"
+  [(set_attr "type" "bitmanip")])
diff --git a/gcc/testsuite/gcc.target/riscv/xtheadbs-tst.c b/gcc/testsuite/gcc.target/riscv/xtheadbs-tst.c
new file mode 100644
index 00000000000..674cec09128
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/xtheadbs-tst.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv32gc_xtheadbs" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc_xtheadbs" { target { rv64 } } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } } */
+
+long
+foo1 (long i)
+{
+  return 1L & (i >> 20);
+}
+
+/* { dg-final { scan-assembler-times "th.tst\t" 1 } } */
+/* { dg-final { scan-assembler-not "andi" } } */