Re: [PATCH 3/5] [RISC-V] Generate Zicond instruction for select pattern with condition eq or neq to 0

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/19/23 04:11, Xiao Zeng wrote:
> This patch completes the recognition of Zicond when the select pattern
> with condition eq or neq to 0 (using equality as an example), namely:
> 
> 1 rd = (rs2 == 0) ? non-imm : 0
> 2 rd = (rs2 == 0) ? non-imm : non-imm
> 3 rd = (rs2 == 0) ? reg : non-imm
> 4 rd = (rs2 == 0) ? reg : reg
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_rtx_costs): IF_THEN_ELSE costs in Zicond.
>          (riscv_expand_conditional_move): Recognize Zicond.
>          * config/riscv/riscv.md: Zicond patterns.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c: New test.
> 	* gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c: New test.
> 	* gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c: New test.
> 	* gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c: New test.
> ---
>   gcc/config/riscv/riscv.cc                     | 125 ++++++++++++++++++
>   gcc/config/riscv/riscv.md                     |   2 +-
>   .../zicond-primitiveSemantics_return_0_imm.c  |  65 +++++++++
>   ...zicond-primitiveSemantics_return_imm_imm.c |  73 ++++++++++
>   ...zicond-primitiveSemantics_return_imm_reg.c |  65 +++++++++
>   ...zicond-primitiveSemantics_return_reg_reg.c |  65 +++++++++
>   6 files changed, 394 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_0_imm.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_imm.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_imm_reg.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics_return_reg_reg.c
> 
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 38d8eb2fcf5..7e6b24bd232 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -2448,6 +2448,17 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
>   	  *total = COSTS_N_INSNS (1);
>   	  return true;
>   	}
> +      else if (TARGET_ZICOND && outer_code == SET &&
> +               ((GET_CODE (XEXP (x, 1)) == REG && XEXP (x, 2) == const0_rtx) ||
> +               (GET_CODE (XEXP (x, 2)) == REG && XEXP (x, 1) == const0_rtx) ||
> +               (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
> +                XEXP (x, 1) == XEXP (XEXP (x, 0), 0)) ||
> +               (GET_CODE (XEXP (x, 1)) == REG && GET_CODE (XEXP (x, 2)) &&
> +                XEXP (x, 2) == XEXP (XEXP (x, 0), 0))))
> +        {
> +          *total = 0;
> +          return true;
> +        }
So why *total = 0.  I would have expected *total = COSTS_N_INSNS (1).


I'm not entirely sure the changes to riscv_expand_conditional_move are 
desirable -- these are likely better done in the generic if-conversion 
pass.


> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 6b8c2e8e268..b4147c7a79c 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -2484,7 +2484,7 @@
>   	(if_then_else:GPR (match_operand 1 "comparison_operator")
>   			  (match_operand:GPR 2 "reg_or_0_operand")
>   			  (match_operand:GPR 3 "sfb_alu_operand")))]
> -  "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV"
> +  "TARGET_SFB_ALU || TARGET_XTHEADCONDMOV || TARGET_ZICOND"
>   {
>     if (riscv_expand_conditional_move (operands[0], operands[1],
>   				     operands[2], operands[3]))
We had to do more than just slap on a TARGET_ZICOND.  I'm a bit 
surprised this worked as-is.  Though we also have bits to handle 
conditions other than eq/ne by first emitting an sCC style insn which 
might be adding complication or cases you hadn't encountered.


Jeff