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

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

On 2023-11-29 13:26  Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
>On 11/27/23 19:32, Fei Gao wrote:
>> op=[PLUS, MINUS, IOR, XOR, ASHIFT, ASHIFTRT, LSHIFTRT, ROTATE, ROTATERT]
>>
>> SIGN_EXTEND, ZERO_EXTEND and SUBREG has been considered
>> to support SImode in 64-bit machine.
>Let's defer these for now.  We're supposed to be wrapping up work that
>was posted before stage1 closed.  If these opcodes were trivial to
>support, then I would let them through, but SUBREGs for example can be
>problematical as their semantics can be complex. 

I can delete the  32bit-support in 64 bit machine.
Or split this patch into 2 parts word-size support and 32bit-support in 64 bit machine.
Or keep current status if the following words persuades you :)

Regarding complexity, the current patch differs from 1st version by "to_replace" and reduces
complexity significantly. noce_simple_bbs() ensures we have only single set for insn_a like x=sext(subreg(y) op subreg(z)).
Instead of constructing an insn considering complex extension and subreg from scratch, to_replace locates the rtx z
in noce_bbs_ok_for_cond_zero_arith,
and then replace it with czero result. In this way, extension and subreg are as simple as reg cases.
All the cases for extension and subreg have been uploaded in this patch.
They can be found by searching "int" in gcc.target/riscv/zicond_ifcvt_opt.c

Could you please let me known which you prefer?

>
>
>>
>> Conditional op, if zero
>> rd = (rc == 0) ? (rs1 op rs2) : rs1
>> -->
>> czero.nez rd, rs2, rc
>> op rd, rs1, rd
>>
>> Conditional op, if non-zero
>> rd = (rc != 0) ? (rs1 op rs2) : rs1
>> -->
>> czero.eqz rd, rs2, rc
>> op rd, rs1, rd
>>
>> Co-authored-by: Xiao Zeng<zengxiao@eswincomputing.com>
>>
>> gcc/ChangeLog:
>>
>>          * ifcvt.cc (noce_try_cond_zero_arith):handler for condtional zero based ifcvt
>>          (noce_emit_czero): helper for noce_try_cond_zero_arith
>>          (noce_cond_zero_binary_op_supported): check supported OPs for condtional zero based ifcvt
>>          (get_base_reg): get base reg of a subreg or the reg itself
>>          (noce_bbs_ok_for_cond_zero_arith): check if BBs are OK for condtional zero based ifcvt
>>          (noce_process_if_block): add noce_try_cond_zero_arith
>>
>> gcc/testsuite/ChangeLog:
>>
>>          * gcc.target/riscv/zicond_ifcvt_opt.c: New test.
>> ---
>>   gcc/ifcvt.cc                                  | 210 ++++++
>>   .../gcc.target/riscv/zicond_ifcvt_opt.c       | 682 ++++++++++++++++++
>>   2 files changed, 892 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..8f6a0e7f5fe 100644
>> --- a/gcc/ifcvt.cc
>> +++ b/gcc/ifcvt.cc
>> @@ -787,6 +787,7 @@ 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 int 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 +1832,40 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>>       return NULL_RTX;
>>   }
>>  
>> +/*  Emit a conditional zero, returning TARGET or NULL_RTX upon failure.
>> +    IF_INFO describes the if-conversion scenario under consideration.
>> +    CZERO_CODE selects the condition (EQ/NE).
>> +    NON_ZERO_OP is the nonzero operand of the conditional move
>> +    TARGET is the desired output register.  */
>> +
>> +static rtx
>> +noce_emit_czero (struct noce_if_info *if_info, enum rtx_code czero_code,
>> +	rtx non_zero_op, rtx target)
>[ ... ]
>The code you wrote is safe in that if constructs a suitable if-then-else
>as a single object, starts a new sequence the uses emit_insn to put that
>object onto a sequence.  Then you extract that one and only one insn
>from the sequence and validate it can be recognized.
>
>In cases where you want to do something like this and know you're going
>to emit one and only one insn you can use 'make_insn_raw' without
>generating a new sequence.
>
>I would suggest you replace all the code starting with start_sequence()
>and ending with end_sequence () (inclusive) with something like
>
>insn = make_insn_raw (set);
>if (recog_memoized (insn) >= 0)
>   {
>     emit_insn (insn);
>     return target;
>   }
>return NULL_RTX; 
Thanks for your advice, I will take it in next version.

>
>
>Note that in the future (gcc-15) when this code is generalized to use
>the expander it will potentially generate multiple insns at which point
>we'll have to put them on a sequence and validate they all are
>recognizable.  But we'll tackle that at the appropriate time. 
Sure. 

>
>
>> +
>> +  return false;
>> +}
>> +
>> +/*  Helper function to return REG itself or inner expression of a SUBREG,
>> +    otherwise NULL_RTX for other RTX_CODE.  */
>> +
>> +static rtx
>> +get_base_reg (rtx exp)
>> +{
>> +  if (REG_P (exp))
>> +    return exp;
>> +  else if (SUBREG_P (exp))
>> +    return SUBREG_REG (exp);
>> +
>> +  return NULL_RTX;
>> +
>I would advise against handling subregs at this point.  What you're
>doing is probably too simplistic given the semantics of subregs.
>
>
>
>> +
>> +  /* Strip sign_extend if any.  */
>> +  if (GET_CODE (a) == SIGN_EXTEND || GET_CODE (a) == ZERO_EXTEND)
>> +    bin_exp = XEXP (a, 0);
>> +  else
>> +    bin_exp = a;
>Similarly while I do think we're going to want to handle extensions,
>let's not try and add them at this point.  We want to get this wrapped
>up & integrated so that everyone can move their focus to bugfixing for
>gcc-14. 
Same reply with the one regarding  *SIGN_EXTEND, ZERO_EXTEND and SUBREG* at
the beginning.

>
>> +
>> +/*  Try to covert if-then-else with conditional zero,
>> +    returning TURE on success or FALSE on failure.
>> +    IF_INFO describes the if-conversion scenario under consideration.  */
>> +
>> +static int
>> +noce_try_cond_zero_arith (struct noce_if_info *if_info)
>> +{
>> +  rtx target, a;
>> +  rtx_insn *seq;
>> +  machine_mode mode = GET_MODE (if_info->x);
>> +  rtx common = NULL_RTX;
>> +  enum rtx_code czero_code = UNKNOWN;
>> +  rtx non_zero_op = NULL_RTX;
>> +  rtx *to_replace = NULL;
>> +
>> +  if (!noce_bbs_ok_for_cond_zero_arith (if_info, &common, &czero_code, &a,
>> +	&to_replace))
>> +    return false;
>> +
>> +  /* Disallow CONST_INT currently for simplicity*/
>> +  if (to_replace == NULL || !REG_P (*to_replace))
>> +    return false;If you're not going to allow CONST_INT reject them in the
>ok_for_condzero_arith routine.  Presumably you're rejecting constants
>because zicond doesn't allow them in the arms.  We'll want to handle
>them in the future and can do so easily once we're using the expander. 

I remembered the shift-by-6 case mentioned by you, see [PATCH 3/4] for const_int support:)
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg327149.html

Also AND is supported  in 
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg327219.html

Could you please reveiw 2 patches above so I will repost together if needed? 
>
>
>
>> +
>> +  non_zero_op = *to_replace;
>> +
>> +  start_sequence ();
>> +
>> +  /* If x is used in both input and out like 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, non_zero_op, target);
>> +  if (!target || !to_replace)
>> +    {
>> +      end_sequence ();
>> +      return false;
>> +    }
>> +
>> +  *to_replace = target;
>> +  emit_insn (gen_rtx_SET (if_info->x, a));
>This isn't necessarily safe.  Use emit_move_insn instead. 
OK. 

Thanks & BR
Fei

>
>This is pretty close.  Hopefully one more iteration and it can be
>integrated.
>
>jeff