Re: [PATCH 5/7] riscv: thead: Add support for XTheadBb ISA extension

["cooper.qu@linux.alibaba.com" <cooper.qu@linux.alibaba.com>]

On Sun, Nov 13, 2022 at 10:46:34PM +0100, Christoph Muellner wrote:
> +(define_expand "extv<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=r")
> +	(sign_extract:GPR (match_operand:GPR 1 "register_operand" "r")
> +			 (match_operand 2 "const_int_operand")
> +			 (match_operand 3 "const_int_operand")))]
> +  "TARGET_XTHEADBB"
> +{
> +  if (TARGET_XTHEADBB
> +      && ((INTVAL (operands[2]) + INTVAL (operands[3]))
> +	  >= GET_MODE_BITSIZE (GET_MODE (operands[1])).to_constant ()))
> +    FAIL;
> +})
> +
> +(define_expand "extzv<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=r")
> +	(zero_extract:GPR (match_operand:GPR 1 "register_operand" "r")
> +			 (match_operand 2 "const_int_operand")
> +			 (match_operand 3 "const_int_operand")))]
> +  "TARGET_XTHEADBB"
> +{
> +  if (TARGET_XTHEADBB
> +      && ((INTVAL (operands[2]) + INTVAL (operands[3]))
> +	  >= GET_MODE_BITSIZE (GET_MODE (operands[1])).to_constant ()))
> +    FAIL;
I doubt whether it is necessary to add this judgment here,
and other architectures seem to have not added it. But there's nothing wrong with adding
> +
> +(define_insn "*th_ext<mode>"
> +  [(set (match_operand:X 0 "register_operand" "=r")
> +	(sign_extract:X (match_operand:X 1 "register_operand" "r")
> +			(match_operand 2 "const_int_operand")
> +			(match_operand 3 "const_int_operand")))]
> +  "TARGET_XTHEADBB"
> +{
> +  operands[3] = GEN_INT (INTVAL (operands[2]) + INTVAL (operands[3]));
> +  return "th.ext\t%0,%1,%2,%3";
> +}
> +  [(set_attr "type" "bitmanip")])
> +
> +(define_insn "*th_extu<mode>"
> +  [(set (match_operand:X 0 "register_operand" "=r")
> +	(zero_extract:X (match_operand:X 1 "register_operand" "r")
> +			(match_operand 2 "const_int_operand")
> +			(match_operand 3 "const_int_operand")))]
> +  "TARGET_XTHEADBB"
> +{
> +  operands[3] = GEN_INT (INTVAL (operands[2]) + INTVAL (operands[3]));
> +  return "th.extu\t%0,%1,%2,%3";
> +}
> +  [(set_attr "type" "bitmanip")])
> +

I think the operands[3] should be:
operands[3] = GEN_INT (INTVAL (operands[2]) + INTVAL (operands[3])) - 1
Because the ext and extu extract the bits %2..%3, when size is 1, the 2%
equals to 3%.
And a small optimization can be done here, the extzv can generate c.andi
when the start bit is 0 and the size is less than 7.

> +/* { dg-final { scan-assembler-times "th.revw\t" 2 } } */
> +/* { dg-final { scan-assembler-times "th.rev\t" 2 } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/xtheadbb-srri.c b/gcc/testsuite/gcc.target/riscv/xtheadbb-srri.c
> new file mode 100644
> index 00000000000..cd992ae3f0a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/xtheadbb-srri.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc_xtheadbb -mabi=lp64" } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-g" } } */
> +
> +unsigned long foo1(unsigned long rs1)
> +{
> +    long shamt = __riscv_xlen - 11;
> +    return (rs1 << shamt) |
> +	   (rs1 >> ((__riscv_xlen - shamt) & (__riscv_xlen - 1)));
> +}
> +unsigned long foo2(unsigned long rs1)
> +{
> +    unsigned long shamt = __riscv_xlen - 11;
> +    return (rs1 >> shamt) |
> +	   (rs1 << ((__riscv_xlen - shamt) & (__riscv_xlen - 1)));
> +}
> +
> +/* { dg-final { scan-assembler-times "th.srri" 2 } } */

Why is there no testcase for ff1 here? It can be generated by the builtin function '__builtin_clzl'.


Thanks,
Cooper