Re: [PATCH 4/5] [ifcvt] optimize x=c ? (y op const_int) : y by RISC-V Zicond like insns

[Jeff Law <jeffreyalaw@gmail.com>]

On 12/5/23 01:12, Fei Gao wrote:
> op=[PLUS, MINUS, IOR, XOR, ASHIFT, ASHIFTRT, LSHIFTRT, ROTATE, ROTATERT, AND]
> 
> Co-authored-by: Xiao Zeng<zengxiao@eswincomputing.com>
> 
> gcc/ChangeLog:
> 
>          * ifcvt.cc (noce_cond_zero_shift_op_supported): check if OP is shift like operation
>          (noce_cond_zero_binary_op_supported): restructure & call noce_cond_zero_shift_op_supported
>          (noce_bbs_ok_for_cond_zero_arith): add support for const_int
>          (noce_try_cond_zero_arith): add support for x=c ? (y op const_int)
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/zicond_ifcvt_opt.c: add TCs for x=c ? (y op const_int) : y



> ---
>   gcc/ifcvt.cc                                  |  45 +-
>   .../gcc.target/riscv/zicond_ifcvt_opt.c       | 774 +++++++++++++++++-
>   2 files changed, 811 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index 29f33f956eb..b84be53ec5c 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -2910,6 +2910,20 @@ noce_try_sign_mask (struct noce_if_info *if_info)
>     return true;
>   }
>   
> +/*  Check if OP is shift-like operation supported by conditional zero
> +    based if conversion, returning TRUE if satisfied otherwise FALSE.
> +
> +    OP is the operation to check.  */
> +static bool
> +noce_cond_zero_shift_op_supported (enum rtx_code op)
> +{
> +  if (op == ASHIFT || op == ASHIFTRT || op == LSHIFTRT || op == ROTATE
> +      || op == ROTATERT)
> +    return true;
Formatting nit.  Go ahead and bring down the || op = ROTATE test as 
well.  That leaves the two lines better balanced with all the shifts on 
one line and all the rotates on another.  It's minor, but we might as 
well keep it easy to read.

> @@ -3089,7 +3111,18 @@ noce_try_cond_zero_arith (struct noce_if_info *if_info)
>   	  return false;
>   	}
>   
> -      *to_replace = target;
> +      if (CONST_INT_P (*to_replace))
> +	{
> +	  if (noce_cond_zero_shift_op_supported (bin_code))
> +	    *to_replace = gen_rtx_SUBREG (E_QImode, target, 0);
> +	  else if (SUBREG_P (bin_op0))
> +	    *to_replace = gen_rtx_SUBREG (GET_MODE (bin_op0), target, 0);
> +	  else
> +	    *to_replace = target;
Not all targets use QImode for their shift counts, so you can't just 
force that argument to QImode.

The way this works in our internal tree is that we re-expand the binary 
operation rather than replacing bits of existing RTL.  That allows the 
expanders to do the right thing automatically for the target WRT 
handling of things like the mode of the shift count.  In fact, I don't 
see how you can ever do replacement of a constant with a register with 
the current scheme since the original constant will be modeless, so you 
never know what mode to use.



Jeff