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

["Christoph Müllner" <christoph.muellner@vrull.eu>]

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