Re: [PATCH] [RISC-V] Fix riscv_expand_conditional_move.

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/27/23 20:21, Die Li wrote:
> Two issues have been observed in current riscv_expand_conditional_move
> implementation.
> 1. Before introduction of TARGET_XTHEADCONDMOV, op0 of comparision expression
> is used for mode comparision with word_mode, but after TARGET_XTHEADCONDMOV
> megered with TARGET_SFB_ALU, dest of if-then-else is used for mode comparision with
> word_mode, and from md file mode of dest is DI or SI which can be different with
> word_mode in RV64.
> 
> 2. TARGET_XTHEADCONDMOV cannot be generated when the mode of the comparison is E_VOID.
> 
> This patch solves the issues above.
> 
> Provide an example from the newly added test case.
> 
> Testcase:
> int ConNmv_reg_reg_reg(int x, int y, int z, int n){
>    if (x != y) return z;
>    return n;
> }
> 
> Cflags:
> -O2 -march=rv64gc_xtheadcondmov -mabi=lp64d
> 
> before patch:
> ConNmv_reg_reg_reg:
> 	bne	a0,a1,.L23
> 	mv	a2,a3
> .L23:
> 	mv	a0,a2
> 	ret
> 
> after patch:
> ConNmv_reg_reg_reg:
> 	sub	a1,a0,a1
> 	th.mveqz	a2,zero,a1
> 	th.mvnez	a3,zero,a1
> 	or	a0,a2,a3
> 	ret
> 
> Co-Authored by: Fei Gao <gaofei@eswincomputing.com>
> Signed-off-by: Die Li <lidie@eswincomputing.com>
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_expand_conditional_move): Fix mode checking.
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/xtheadcondmov-indirect-rv32.c: New test.
>          * gcc.target/riscv/xtheadcondmov-indirect-rv64.c: New test.
> ---
>   gcc/config/riscv/riscv.cc                     |   4 +-
>   .../riscv/xtheadcondmov-indirect-rv32.c       | 116 ++++++++++++++++++
>   .../riscv/xtheadcondmov-indirect-rv64.c       | 116 ++++++++++++++++++
>   3 files changed, 234 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadcondmov-indirect-rv32.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadcondmov-indirect-rv64.c
> 
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 1529855a2b4..30ace45dc5f 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -3411,7 +3411,7 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt)
>         && GET_MODE_CLASS (mode) == MODE_INT
>         && reg_or_0_operand (cons, mode)
>         && reg_or_0_operand (alt, mode)
> -      && GET_MODE (op) == mode
> +      && (GET_MODE (op) == mode || GET_MODE (op) == E_VOIDmode)
So I nearly suggested we just drop this check.  In general comparisons 
don't have modes.  But I don't think it's going to hurt and it lines up 
with the predicates that test for conditions.

Note that some of the new tests are still failing (though they certainly 
do much better after your patch)
.
>   FAIL: gcc.target/riscv/xtheadcondmov-indirect-rv32.c   -O1   check-function-bodies ConNmv_imm_imm_r >   FAIL: gcc.target/riscv/xtheadcondmov-indirect-rv32.c   -O2 
check-function-bodies ConNmv_imm_imm_reg
>   FAIL: gcc.target/riscv/xtheadcondmov-indirect-rv32.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none   check-function-bodies ConNmv_imm_imm_reg
>   FAIL: gcc.target/riscv/xtheadcondmov-indirect-rv32.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects   check-function-bodies ConNmv_imm_imm_reg
>   FAIL: gcc.target/riscv/xtheadcondmov-indirect-rv32.c   -O3 -g   check-function-bodies ConNmv_imm_imm_reg


[ ... and a few more instances omitted ... ]

I went ahead and pushed the patch, but you might want to double-check 
the state of those failing tests.

Jeff