Re: [PATCH] RISC-V: Optimize TARGET_XTHEADCONDMOV

[Jeff Law <jeffreyalaw@gmail.com>]

On 5/25/23 18:52, Die Li wrote:
> This patch allows less instructions to be used when TARGET_XTHEADCONDMOV is enabled.
> 
> Provide an example from the existing testcases.
> 
> Testcase:
> int ConEmv_imm_imm_reg(int x, int y){
>    if (x == 1000) return 10;
>    return y;
> }
> 
> Cflags:
> -O2 -march=rv64gc_xtheadcondmov -mabi=lp64d
> 
> before patch:
> ConEmv_imm_imm_reg:
> 	addi	a5,a0,-1000
> 	li	a0,10
> 	th.mvnez	a0,zero,a5
> 	th.mveqz	a1,zero,a5
> 	or	a0,a0,a1
> 	ret
> 
> after patch:
> ConEmv_imm_imm_reg:
> 	addi	a5,a0,-1000
> 	li	a0,10
> 	th.mvnez	a0,a1,a5
> 	ret
> 
> Signed-off-by: Die Li <lidie@eswincomputing.com>
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_expand_conditional_move_onesided): Delete.
>          (riscv_expand_conditional_move):  Reuse the TARGET_SFB_ALU expand process for TARGET_XTHEADCONDMOV
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/xtheadcondmov-indirect-rv32.c: Update the output.
>          * gcc.target/riscv/xtheadcondmov-indirect-rv64.c: Likewise.
I've made minor formatting adjustments and pushed this to the trunk.


> @@ -3492,14 +3462,12 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt)
>         && GET_MODE (op0) == mode
>         && GET_MODE (op1) == mode
>         && (code == EQ || code == NE))
> +        need_eq_ne_p = true;
So the need_eq_ne_p should have indented 2 spaces in from the IF statement.

> +
> +  if (need_eq_ne_p || (TARGET_SFB_ALU
> +	   && GET_MODE (op0) == word_mode))This should have been:
   if (need_eq_ne_p
       || (TARGET_SFB_ALU && GET_MODE (op0) == word_mode))

I've fixed this as well.

I would recommend you review the GCC coding guidelines.  While I fixed 
the problems this time, I'll likely ask you to do so in the future.

jeff