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 19:09  Fei Gao <gaofei@eswincomputing.com> wrote:
>
>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? 
I split the patches. See new series https://patchwork.sourceware.org/project/gcc/list/?series=27924
>
>>
>>
>>>
>>> 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. 
I used add_insn instead of emit_insn to avoid a segment fault caused by invalid
NEXT_INSN (insn) of the raw insn.

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