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

["Fei Gao" <gaofei@eswincomputing.com>]

On 2023-11-20 15:10  Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>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. 

Done.

>
>
>
>
>> +  if (opcode == AND)
>> +    {
>> +      tmp
>> +	= expand_simple_binop (mode, AND, common, z, NULL_RTX, 0, OPTAB_DIRECT);
>OPTAB_WIDEN here I think. 
I restructured the codes to have a simple implementation. But AND is different from 
others operations in czero based ifcvt, I kept the ugly codes locally. I will refine it and post
if current codes accepted.

>
>
>> +      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;
>} 
I tried both in old and new series. Zicond insns could be generated.

BR, 
Fei
>
>
>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