Re: [PATCH] RISC-V: Add bext pattern for ZBS

[Jeff Law <jeffreyalaw@gmail.com>]

On 5/4/23 11:08, Raphael Moreira Zinsly wrote:
> When (a & (1 << bit_no)) is tested inside an IF we can use a bit extract.
> 
> 	gcc/ChangeLog:
> 
> 		* config/riscv/bitmanip.md
> 		(bext<mode>): Rename one to avoid name clash.
> 		(branch<X:mode>_bext): New split pattern.
> 
> 	gcc/testsuite/ChangeLog:
> 		* gcc.target/riscv/zbs-bext-02.c: New test.


> ---
>   gcc/config/riscv/bitmanip.md                 | 24 +++++++++++++++++++-
>   gcc/testsuite/gcc.target/riscv/zbs-bext-02.c | 18 +++++++++++++++
>   2 files changed, 41 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zbs-bext-02.c
> 
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index a27fc3e34a1..e29e2d1fa53 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -595,7 +595,7 @@
>   ;; When performing `(a & (1UL << bitno)) ? 0 : -1` the combiner
>   ;; usually has the `bitno` typed as X-mode (i.e. no further
>   ;; zero-extension is performed around the bitno).
> -(define_insn "*bext<mode>"
> +(define_insn "*bext<mode>_2"
>     [(set (match_operand:X 0 "register_operand" "=r")
>   	(zero_extract:X (match_operand:X 1 "register_operand" "r")
>   			(const_int 1)
This doesn't make sense to me.  Is it possible this was from an earlier 
version of the patch?

In general when we have  a * prefix, we're allowed to have multiple 
patterns with the same name.  Essentially the pattern names are just for 
debugging purposes, no API is exposed to generate those patterns when 
there's a '*' prefix.


> @@ -720,6 +720,28 @@
>      operands[9] = GEN_INT (clearbit);
>   })
>   
> +;; IF_THEN_ELSE: test for (a & (1 << BIT_NO))
> +(define_insn_and_split "*branch<X:mode>_bext"
> +  [(set (pc)
> +	(if_then_else
> +	  (match_operator 1 "equality_operator"
> +        [(zero_extract:X (match_operand:X 2 "register_operand" "r")
> +                (const_int 1)
> +                (zero_extend:X (match_operand:QI 3 "register_operand" "r")))
> +	    (const_int 0)])
> +	 (label_ref (match_operand 0 "" ""))
> +	 (pc)))
> +   (clobber (match_scratch:X 4 "=&r"))]
Formatting nit.  In general the operands of a rtx operator all line up 
together when we can.  So in this case the (const_int 1) should line up 
under the (match_operand:X 2).  Similarly for the (zero_extend:X).  That 
may require wrapping the zero_extned line.  The way to do that would be 
to bring its match_operand down to a new line, indent it two spaces from 
the open paren of the (zero_extend.



It's been a while since we poked at this, so maybe you've already told 
me before, but would it make sense to use the GPR iterator rather than 
the X iterator?

GPR would result in two patterns that are available to match at the same 
time, one for SI, another for DI.

X also results in two patterns, but only one is available at any given 
time dependent on TARGET_64BIT.

I guess the rest are defined in terms of X, particularly the bext<mode> 
pattern.  So nevermind, keep it as X.

So I think the only things we potentially adjust is to remove the hunk 
which changes the name of the *bext<mode> pattern and the whitespace 
fix.  I think we'll be good to go after those changes.

Jeff