Re: [PATCH 4/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:

> diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> index 6e341fc4d4b..cfa9bc4b850 100644
> --- a/gcc/ifcvt.cc
> +++ b/gcc/ifcvt.cc
> @@ -2911,7 +2911,7 @@ noce_try_sign_mask (struct noce_if_info *if_info)
>   static bool
>   noce_cond_zero_binary_op_supported (enum rtx_code op)
>   {
> -  if (op == PLUS || op == MINUS || op == IOR || op == XOR)
> +  if (op == PLUS || op == MINUS || op == IOR || op == XOR || op == AND)
>       return true;
Include ASHIFT, LSHIFTRT, ASHIFTRT, ROTATE, ROTATERT.  That should pick 
up that critical conditional-shift-by-6 in leela.




> +  if (opcode == AND)
> +    {
> +      tmp
> +	= expand_simple_binop (mode, AND, common, z, NULL_RTX, 0, OPTAB_DIRECT);
OPTAB_WIDEN here I think.


> +      if (!tmp)
> +	{
> +	  end_sequence ();
> +	  return FALSE;
> +	}
>   
> -  /* 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;
> +      target = noce_emit_czero (if_info, czero_code, common, if_info->x);
> +      if (!target)
> +	{
> +	  end_sequence ();
> +	  return FALSE;
Please try to be consistent with upper/lower case.  In your prior 
patches you used lower case for true/false.  In this patch you're using 
upper case.  Lower case seems to be the standard in that file, so use 
lower case.

> +	}
>   
> -  target = noce_emit_czero (if_info, czero_code, z, target);
> -  if (!target)
> -    {
> -      end_sequence ();
> -      return false;
> +      target = expand_simple_binop (mode, IOR, tmp, target, if_info->x, 0,
> +				    OPTAB_DIRECT);
>       }
> +  else
> +    {
> +      /* 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;
As noted before you may not be able to generate a new register when 
ifcvt is run after register allocation.  Your code needs to handle that 
correctly.


> +
> +      target = noce_emit_czero (if_info, czero_code, z, target);
> +      if (!target)
> +	{
> +	  end_sequence ();
> +	  return false;
> +	}
>   
> -  target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0,
> -				OPTAB_DIRECT);
> +      target = expand_simple_binop (mode, opcode, common, target, if_info->x, 0,
> +				    OPTAB_DIRECT);
OPTAB_WIDEN.

And the usual comments about avoiding explicit registers in the tests.


I would suggest you try to handle this case as well, I don't think it's 
handled by your current code:

long
eq2 (long a, long b)
{
   if (a == 0)
     return b;

   return 0;
}


There's probably also a negated version of that to be handled as well.


Overall I think we can go forward with your patches after things are 
fixed.  I'm inclined to wait until after Maciej has integrated his 
changes before actually committing them.  While I don't expect problems, 
I wouldn't want Maciej to have to respin a 40+ patch series.

Note that while we transition to stage3 development today, your patch 
was posted while we were in stage1, so you've met the deadline.  We just 
need to get the updates done relatively soon rather than having it drag 
late into stage3.

Jeff