Re: [PATCH 2/4] [ifcvt] if convert x=c ? y+z : y by RISC-V Zicond like insns

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/30/23 01:25, Fei Gao wrote:
> Conditional add, if zero
> rd = (rc == 0) ? (rs1 + rs2) : rs1
> -->
> czero.nez rd, rs2, rc
> add rd, rs1, rd
> 
> Conditional add, if non-zero
> rd = (rc != 0) ? (rs1 + rs2) : rs1
> -->
> czero.eqz rd, rs2, rc
> add rd, rs1, rd
> 
> Co-authored-by: Xiao Zeng<zengxiao@eswincomputing.com>
> 
> gcc/ChangeLog:
> 
>          * ifcvt.cc (noce_emit_czero): helper for noce_try_cond_zero_arith
>          (noce_try_cond_zero_arith): handler for condtional zero op
>          (noce_process_if_block): add noce_try_cond_zero_arith with hook control
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/zicond_ifcvt_opt.c: New test.
> ---
>   gcc/ifcvt.cc                                  | 112 +++++++++++++++
>   .../gcc.target/riscv/zicond_ifcvt_opt.c       | 130 ++++++++++++++++++
>   2 files changed, 242 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c
> 
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index a0af553b9ff..4f98c1c7bf9 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -781,12 +781,14 @@ static bool noce_try_store_flag_constants (struct noce_if_info *);
>   static bool noce_try_store_flag_mask (struct noce_if_info *);
>   static rtx noce_emit_cmove (struct noce_if_info *, rtx, enum rtx_code, rtx,
>   			    rtx, rtx, rtx, rtx = NULL, rtx = NULL);
> +static rtx noce_emit_czero (struct noce_if_info *, enum rtx_code, rtx, rtx);
>   static bool noce_try_cmove (struct noce_if_info *);
>   static bool noce_try_cmove_arith (struct noce_if_info *);
>   static rtx noce_get_alt_condition (struct noce_if_info *, rtx, rtx_insn **);
>   static bool noce_try_minmax (struct noce_if_info *);
>   static bool noce_try_abs (struct noce_if_info *);
>   static bool noce_try_sign_mask (struct noce_if_info *);
> +static bool noce_try_cond_zero_arith (struct noce_if_info *);
>   
>   /* Return the comparison code for reversed condition for IF_INFO,
>      or UNKNOWN if reversing the condition is not possible.  */
> @@ -1831,6 +1833,32 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>       return NULL_RTX;
>   }
>   
> +static rtx
> +noce_emit_czero (struct noce_if_info *if_info, enum rtx_code czero_code, rtx z, rtx target)
Every function needs a comment describing what the function does, it's 
return value(s) and its arguments.  There are many examples in ifcvt.cc 
you can use to guide you.  I might start with something like this:

/* Emit a conditional zero, returning the location of the result
    or NULL_RTX upon failure.

    IF_INFO describes the if-conversion scenario under consideration.
    CZERO_CODE selects the condition (EQ/NE).
    Z is the nonzero operand of the conditional move
    TARGET is the desired output register.  */

Or something like that.  I would suggest renaming "Z" to something more 
meaningful.



>   
> +/* Convert x = c ? y + z : y or x = c ? y : y + z. */
> +
> +static bool
> +noce_try_cond_zero_arith (struct noce_if_info *if_info)
The function comment really should be improved.  For example it doesn't 
indicate what the return value is.

> +
> +  /* cond must be EQ or NEQ comparision of a reg and 0.  */
In general when you refer to a variable in a comment, do so in upper 
case.  Use NE rather than NEQ as the former is how most code refers to a 
not-equal rtx code.


> +  if (GET_CODE (cond) != NE && GET_CODE (cond) != EQ)
> +    return false;
> +  if (!REG_P (XEXP (cond, 0)) || !rtx_equal_p (XEXP (cond, 1), const0_rtx))
> +    return false;
> +
> +  /* check y + z:y*/
> +  if (GET_CODE (a) == PLUS && REG_P (XEXP (a, 0)) && REG_P (XEXP (a, 1))
> +      && REG_P (b) && rtx_equal_p (XEXP (a, 0), b))
Write comments as complete sentences.

> +    {
> +      common = b;
> +      z = XEXP (a, 1);
Rather than "z" use a more descriptive variable name.


> +
> +  /* If we have x = c ? x + z : x, use a new reg to avoid modifying x  */
> +  if (common && rtx_equal_p (common, if_info->x))
> +    target = gen_reg_rtx (mode);
> +  else
> +    target = if_info->x;
if-conversion runs both before and after register allocation.  So you 
have to handle the case where you can not generate new registers.  Use 
can_create_pseudo_p () to test for that.  You may need to fail if you 
can't generate a new register.

> +
> +  target = noce_emit_czero (if_info, czero_code, z, target);
> +  if (!target)
> +    {
> +      end_sequence ();
> +      return false;
> +    }
> +
> +  target = expand_simple_binop (mode, PLUS, common, target, if_info->x, 0,
> +				OPTAB_DIRECT);
I think you want OPTAB_WIDEN and in the other calls.

> @@ -3975,6 +4085,8 @@ noce_process_if_block (struct noce_if_info *if_info)
>   	goto success;
>         if (noce_try_store_flag_mask (if_info))
>   	goto success;
> +      if (targetm.have_cond_zero () && noce_try_cond_zero_arith (if_info))
> +	goto success;
Replace targetm.have_cond_zero with HAVE_conditional_move since that's 
the RTL primitive we're building from.


>
> +**test_ADD_ceqz:
> +**	czero\.eqz	a3,a2,a3
> +**	add	a0,a1,a3
> +**	ret
Please don't use explicit registers unless you know they will always be 
correct.  In this sequence there's no guarantee the register allocator 
will put the result of the czero.eqz into $a3.  Use a suitable regexp 
instead to match a variety of registers.  This will be an issue for all 
your new tests.



Jeff