[PATCH] RISC-V: Refactor and clean expand_cond_len_{unop,binop,ternop}

[PATCH] RISC-V: Refactor and clean expand_cond_len_{unop,binop,ternop}

[Lehua Ding]

Hi,

This patch refactors the codes of expand_cond_len_{unop,binop,ternop}.
Introduces a new unified function expand_cond_len_op to do the main thing.
The expand_cond_len_{unop,binop,ternop} functions only care about how
to pass the operands to the intrinsic patterns.

Best,
Lehua

gcc/ChangeLog:

	* config/riscv/autovec.md: Adjust
	* config/riscv/riscv-protos.h (RVV_VUNDEF): Clean.
	(get_vlmax_rtx): Exported.
	* config/riscv/riscv-v.cc (emit_nonvlmax_fp_ternary_tu_insn): Deleted.
	(emit_vlmax_masked_gather_mu_insn): Adjust.
	(get_vlmax_rtx): New func.
	(expand_load_store): Adjust.
	(expand_cond_len_unop): Call expand_cond_len_op.
	(expand_cond_len_op): New subroutine.
	(expand_cond_len_binop): Call expand_cond_len_op.
	(expand_cond_len_ternop): Call expand_cond_len_op.
	(expand_lanes_load_store): Adjust.

---
 gcc/config/riscv/autovec.md     |   6 +-
 gcc/config/riscv/riscv-protos.h |  16 ++-
 gcc/config/riscv/riscv-v.cc     | 166 ++++++++++----------------------
 3 files changed, 60 insertions(+), 128 deletions(-)

diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md
index e9659b2b157..ad072ff439a 100644
--- a/gcc/config/riscv/autovec.md
+++ b/gcc/config/riscv/autovec.md
@@ -971,9 +971,9 @@
   rtx mask = gen_reg_rtx (mask_mode);
   riscv_vector::expand_vec_cmp (mask, LT, operands[1], zero);
 
-  rtx ops[] = {operands[0], mask, operands[1], operands[1]};
-  riscv_vector::emit_vlmax_masked_mu_insn (code_for_pred (NEG, <MODE>mode),
-					   riscv_vector::RVV_UNOP_MU, ops);
+  rtx ops[] = {operands[0], mask, operands[1], operands[1],
+               riscv_vector::get_vlmax_rtx (<MODE>mode)};
+  riscv_vector::expand_cond_len_unop (NEG, ops);
   DONE;
 })
 
diff --git a/gcc/config/riscv/riscv-protos.h b/gcc/config/riscv/riscv-protos.h
index 2c4405c9860..6a49718b34d 100644
--- a/gcc/config/riscv/riscv-protos.h
+++ b/gcc/config/riscv/riscv-protos.h
@@ -180,25 +180,20 @@ namespace riscv_vector {
 #define RVV_VUNDEF(MODE)                                                       \
   gen_rtx_UNSPEC (MODE, gen_rtvec (1, gen_rtx_REG (SImode, X0_REGNUM)),        \
 		  UNSPEC_VUNDEF)
+
+/* The value means the number of operands for insn_expander.  */
 enum insn_type
 {
   RVV_MISC_OP = 1,
   RVV_UNOP = 2,
-  RVV_UNOP_M = RVV_UNOP + 2,
-  RVV_UNOP_MU = RVV_UNOP + 2,
-  RVV_UNOP_TU = RVV_UNOP + 2,
-  RVV_UNOP_TUMU = RVV_UNOP + 2,
+  RVV_UNOP_MASK = RVV_UNOP + 2,
   RVV_BINOP = 3,
-  RVV_BINOP_MU = RVV_BINOP + 2,
-  RVV_BINOP_TU = RVV_BINOP + 2,
-  RVV_BINOP_TUMU = RVV_BINOP + 2,
+  RVV_BINOP_MASK = RVV_BINOP + 2,
   RVV_MERGE_OP = 4,
   RVV_CMP_OP = 4,
   RVV_CMP_MU_OP = RVV_CMP_OP + 2, /* +2 means mask and maskoff operand.  */
   RVV_TERNOP = 5,
-  RVV_TERNOP_MU = RVV_TERNOP + 1,
-  RVV_TERNOP_TU = RVV_TERNOP + 1,
-  RVV_TERNOP_TUMU = RVV_TERNOP + 1,
+  RVV_TERNOP_MASK = RVV_TERNOP + 1,
   RVV_WIDEN_TERNOP = 4,
   RVV_SCALAR_MOV_OP = 4, /* +1 for VUNDEF according to vector.md.  */
   RVV_SLIDE_OP = 4,      /* Dest, VUNDEF, source and offset.  */
@@ -258,6 +253,7 @@ void emit_vlmax_masked_mu_insn (unsigned, int, rtx *);
 void emit_scalar_move_insn (unsigned, rtx *, rtx = 0);
 void emit_nonvlmax_integer_move_insn (unsigned, rtx *, rtx);
 enum vlmul_type get_vlmul (machine_mode);
+rtx get_vlmax_rtx (machine_mode);
 unsigned int get_ratio (machine_mode);
 unsigned int get_nf (machine_mode);
 machine_mode get_subpart_mode (machine_mode);
diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 14eda581d00..8b5bb0097bc 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -759,28 +759,6 @@ emit_vlmax_fp_ternary_insn (unsigned icode, int op_num, rtx *ops, rtx vl)
   e.emit_insn ((enum insn_code) icode, ops);
 }
 
-/* This function emits a {NONVLMAX, TAIL_UNDISTURBED, MASK_ANY} vsetvli followed
- * by the ternary operation which always has a real merge operand.  */
-static void
-emit_nonvlmax_fp_ternary_tu_insn (unsigned icode, int op_num, rtx *ops, rtx vl)
-{
-  machine_mode dest_mode = GET_MODE (ops[0]);
-  machine_mode mask_mode = get_mask_mode (dest_mode);
-  insn_expander<RVV_INSN_OPERANDS_MAX> e (/*OP_NUM*/ op_num,
-					  /*HAS_DEST_P*/ true,
-					  /*FULLY_UNMASKED_P*/ false,
-					  /*USE_REAL_MERGE_P*/ true,
-					  /*HAS_AVL_P*/ true,
-					  /*VLMAX_P*/ false,
-					  /*DEST_MODE*/ dest_mode,
-					  /*MASK_MODE*/ mask_mode);
-  e.set_policy (TAIL_UNDISTURBED);
-  e.set_policy (MASK_ANY);
-  e.set_rounding_mode (FRM_DYN);
-  e.set_vl (vl);
-  e.emit_insn ((enum insn_code) icode, ops);
-}
-
 /* This function emits a {NONVLMAX, TAIL_ANY, MASK_ANY} vsetvli followed by the
  * actual operation.  */
 void
@@ -1165,8 +1143,8 @@ emit_vlmax_masked_gather_mu_insn (rtx target, rtx op, rtx sel, rtx mask)
     }
   else
     icode = code_for_pred_gather (data_mode);
-  rtx ops[] = {target, mask, target, op, sel};
-  emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MU, ops);
+  rtx ops[RVV_BINOP_MASK] = {target, mask, target, op, sel};
+  emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MASK, ops);
 }
 
 /* According to RVV ISA spec (16.5.1. Synthesizing vdecompress):
@@ -1796,6 +1774,14 @@ get_vlmul (machine_mode mode)
   return mode_vtype_infos.vlmul[mode];
 }
 
+/* Return the VLMAX rtx of vector mode MODE.  */
+rtx
+get_vlmax_rtx (machine_mode mode)
+{
+  gcc_assert (riscv_v_ext_vector_mode_p (mode));
+  return gen_int_mode (GET_MODE_NUNITS (mode), Pmode);
+}
+
 /* Return the NF value of the corresponding mode.  */
 unsigned int
 get_nf (machine_mode mode)
@@ -3353,7 +3339,7 @@ expand_load_store (rtx *ops, bool is_load)
       if (is_load)
 	{
 	  rtx m_ops[] = {ops[0], mask, RVV_VUNDEF (mode), ops[1]};
-	  emit_vlmax_masked_insn (code_for_pred_mov (mode), RVV_UNOP_M, m_ops);
+	  emit_vlmax_masked_insn (code_for_pred_mov (mode), RVV_UNOP_MASK, m_ops);
 	}
       else
 	{
@@ -3370,7 +3356,7 @@ expand_load_store (rtx *ops, bool is_load)
       if (is_load)
 	{
 	  rtx m_ops[] = {ops[0], mask, RVV_VUNDEF (mode), ops[1]};
-	  emit_nonvlmax_masked_insn (code_for_pred_mov (mode), RVV_UNOP_M,
+	  emit_nonvlmax_masked_insn (code_for_pred_mov (mode), RVV_UNOP_MASK,
 				     m_ops, len);
 	}
       else
@@ -3389,32 +3375,26 @@ needs_fp_rounding (rtx_code code, machine_mode mode)
   return code != SMIN && code != SMAX && code != NEG && code != ABS;
 }
 
-/* Expand unary ops COND_LEN_*.  */
-void
-expand_cond_len_unop (rtx_code code, rtx *ops)
+/* Subroutine to expand COND_LEN_* patterns.  */
+static void
+expand_cond_len_op (rtx_code code, unsigned icode, int op_num, rtx *cond_ops,
+		    rtx len)
 {
-  rtx dest = ops[0];
-  rtx mask = ops[1];
-  rtx src = ops[2];
-  rtx merge = ops[3];
-  rtx len = ops[4];
+  rtx dest = cond_ops[0];
+  rtx mask = cond_ops[1];
   machine_mode mode = GET_MODE (dest);
   machine_mode mask_mode = GET_MODE (mask);
-
   poly_int64 value;
   bool is_dummy_mask = rtx_equal_p (mask, CONSTM1_RTX (mask_mode));
   bool is_vlmax_len
     = poly_int_rtx_p (len, &value) && known_eq (value, GET_MODE_NUNITS (mode));
-  rtx cond_ops[] = {dest, mask, merge, src};
-  insn_code icode = code_for_pred (code, mode);
-
   if (is_dummy_mask)
     {
       /* Use TU, MASK ANY policy.  */
       if (needs_fp_rounding (code, mode))
-	emit_nonvlmax_fp_tu_insn (icode, RVV_UNOP_TU, cond_ops, len);
+	emit_nonvlmax_fp_tu_insn (icode, op_num, cond_ops, len);
       else
-	emit_nonvlmax_tu_insn (icode, RVV_UNOP_TU, cond_ops, len);
+	emit_nonvlmax_tu_insn (icode, op_num, cond_ops, len);
     }
   else
     {
@@ -3422,21 +3402,37 @@ expand_cond_len_unop (rtx_code code, rtx *ops)
 	{
 	  /* Use TAIL ANY, MU policy.  */
 	  if (needs_fp_rounding (code, mode))
-	    emit_vlmax_masked_fp_mu_insn (icode, RVV_UNOP_MU, cond_ops);
+	    emit_vlmax_masked_fp_mu_insn (icode, op_num, cond_ops);
 	  else
-	    emit_vlmax_masked_mu_insn (icode, RVV_UNOP_MU, cond_ops);
+	    emit_vlmax_masked_mu_insn (icode, op_num, cond_ops);
 	}
       else
 	{
 	  /* Use TU, MU policy.  */
 	  if (needs_fp_rounding (code, mode))
-	    emit_nonvlmax_fp_tumu_insn (icode, RVV_UNOP_TUMU, cond_ops, len);
+	    emit_nonvlmax_fp_tumu_insn (icode, op_num, cond_ops, len);
 	  else
-	    emit_nonvlmax_tumu_insn (icode, RVV_UNOP_TUMU, cond_ops, len);
+	    emit_nonvlmax_tumu_insn (icode, op_num, cond_ops, len);
 	}
     }
 }
 
+/* Expand unary ops COND_LEN_*.  */
+void
+expand_cond_len_unop (rtx_code code, rtx *ops)
+{
+  rtx dest = ops[0];
+  rtx mask = ops[1];
+  rtx src = ops[2];
+  rtx merge = ops[3];
+  rtx len = ops[4];
+
+  machine_mode mode = GET_MODE (dest);
+  insn_code icode = code_for_pred (code, mode);
+  rtx cond_ops[RVV_UNOP_MASK] = {dest, mask, merge, src};
+  expand_cond_len_op (code, icode, RVV_UNOP_MASK, cond_ops, len);
+}
+
 /* Expand binary ops COND_LEN_*.  */
 void
 expand_cond_len_binop (rtx_code code, rtx *ops)
@@ -3447,43 +3443,11 @@ expand_cond_len_binop (rtx_code code, rtx *ops)
   rtx src2 = ops[3];
   rtx merge = ops[4];
   rtx len = ops[5];
-  machine_mode mode = GET_MODE (dest);
-  machine_mode mask_mode = GET_MODE (mask);
 
-  poly_int64 value;
-  bool is_dummy_mask = rtx_equal_p (mask, CONSTM1_RTX (mask_mode));
-  bool is_vlmax_len
-    = poly_int_rtx_p (len, &value) && known_eq (value, GET_MODE_NUNITS (mode));
-  rtx cond_ops[] = {dest, mask, merge, src1, src2};
+  machine_mode mode = GET_MODE (dest);
   insn_code icode = code_for_pred (code, mode);
-
-  if (is_dummy_mask)
-    {
-      /* Use TU, MASK ANY policy.  */
-      if (needs_fp_rounding (code, mode))
-	emit_nonvlmax_fp_tu_insn (icode, RVV_BINOP_TU, cond_ops, len);
-      else
-	emit_nonvlmax_tu_insn (icode, RVV_BINOP_TU, cond_ops, len);
-    }
-  else
-    {
-      if (is_vlmax_len)
-	{
-	  /* Use TAIL ANY, MU policy.  */
-	  if (needs_fp_rounding (code, mode))
-	    emit_vlmax_masked_fp_mu_insn (icode, RVV_BINOP_MU, cond_ops);
-	  else
-	    emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MU, cond_ops);
-	}
-      else
-	{
-	  /* Use TU, MU policy.  */
-	  if (needs_fp_rounding (code, mode))
-	    emit_nonvlmax_fp_tumu_insn (icode, RVV_BINOP_TUMU, cond_ops, len);
-	  else
-	    emit_nonvlmax_tumu_insn (icode, RVV_BINOP_TUMU, cond_ops, len);
-	}
-    }
+  rtx cond_ops[RVV_BINOP_MASK] = {dest, mask, merge, src1, src2};
+  expand_cond_len_op (code, icode, RVV_BINOP_MASK, cond_ops, len);
 }
 
 /* Prepare insn_code for gather_load/scatter_store according to
@@ -3649,42 +3613,14 @@ expand_cond_len_ternop (unsigned icode, rtx *ops)
 {
   rtx dest = ops[0];
   rtx mask = ops[1];
+  rtx src1 = ops[2];
+  rtx src2 = ops[3];
+  rtx src3 = ops[4];
+  rtx merge = ops[5];
   rtx len = ops[6];
-  machine_mode mode = GET_MODE (dest);
-  machine_mode mask_mode = GET_MODE (mask);
-
-  poly_int64 value;
-  bool is_dummy_mask = rtx_equal_p (mask, CONSTM1_RTX (mask_mode));
-  bool is_vlmax_len
-    = poly_int_rtx_p (len, &value) && known_eq (value, GET_MODE_NUNITS (mode));
 
-  if (is_dummy_mask)
-    {
-      /* Use TU, MASK ANY policy.  */
-      if (FLOAT_MODE_P (mode))
-	emit_nonvlmax_fp_ternary_tu_insn (icode, RVV_TERNOP_TU, ops, len);
-      else
-	emit_nonvlmax_tu_insn (icode, RVV_TERNOP_TU, ops, len);
-    }
-  else
-    {
-      if (is_vlmax_len)
-	{
-	  /* Use TAIL ANY, MU policy.  */
-	  if (FLOAT_MODE_P (mode))
-	    emit_vlmax_masked_fp_mu_insn (icode, RVV_TERNOP_MU, ops);
-	  else
-	    emit_vlmax_masked_mu_insn (icode, RVV_TERNOP_MU, ops);
-	}
-      else
-	{
-	  /* Use TU, MU policy.  */
-	  if (FLOAT_MODE_P (mode))
-	    emit_nonvlmax_fp_tumu_insn (icode, RVV_TERNOP_TUMU, ops, len);
-	  else
-	    emit_nonvlmax_tumu_insn (icode, RVV_TERNOP_TUMU, ops, len);
-	}
-    }
+  rtx cond_ops[RVV_TERNOP_MASK] = {dest, mask, src1, src2, src3, merge};
+  expand_cond_len_op (UNSPEC, icode, RVV_TERNOP_MASK, cond_ops, len);
 }
 
 /* Expand reduction operations.  */
@@ -3790,7 +3726,7 @@ expand_lanes_load_store (rtx *ops, bool is_load)
 	{
 	  rtx m_ops[] = {reg, mask, RVV_VUNDEF (mode), addr};
 	  emit_vlmax_masked_insn (code_for_pred_unit_strided_load (mode),
-				  RVV_UNOP_M, m_ops);
+				  RVV_UNOP_MASK, m_ops);
 	}
       else
 	{
@@ -3808,7 +3744,7 @@ expand_lanes_load_store (rtx *ops, bool is_load)
 	{
 	  rtx m_ops[] = {reg, mask, RVV_VUNDEF (mode), addr};
 	  emit_nonvlmax_masked_insn (code_for_pred_unit_strided_load (mode),
-				     RVV_UNOP_M, m_ops, len);
+				     RVV_UNOP_MASK, m_ops, len);
 	}
       else
 	emit_insn (gen_pred_unit_strided_store (mode, mask, addr, reg, len,
-- 
2.36.3

Re: [PATCH] RISC-V: Refactor and clean expand_cond_len_{unop,binop,ternop}

[Robin Dapp]

Hi Lehua,

thanks for starting with the refactoring.  I have some minor comments.

> +/* The value means the number of operands for insn_expander.  */
>  enum insn_type
>  {
>    RVV_MISC_OP = 1,
>    RVV_UNOP = 2,
> -  RVV_UNOP_M = RVV_UNOP + 2,
> -  RVV_UNOP_MU = RVV_UNOP + 2,
> -  RVV_UNOP_TU = RVV_UNOP + 2,
> -  RVV_UNOP_TUMU = RVV_UNOP + 2,
> +  RVV_UNOP_MASK = RVV_UNOP + 2,

Cleanup up here is good, right now it's not really an insn_type but
indeed just the number of operands.  My original idea was to have an
insn type and a mostly unified expander that performs all necessary
operations depending on the insn_type.  Just to give an idea of why it's
called that way.

> +  rtx ops[RVV_BINOP_MASK] = {target, mask, target, op, sel};
> +  emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MASK, ops);

One of the ideas was that a function emit_vlmax_masked_mu_insn would already
know that it's dealing with a mask and we would just pass something like
RVV_BINOP.  The other way would be to just have emit_vlmax_mu_insn or
something and let the rest be deduced from the insn_type.  Even the vlmax
I intended to have mostly implicit but that somehow got lost during
refactorings :)  No need to change anything for now, just for perspective
again. 

> -/* Expand unary ops COND_LEN_*.  */
> -void
> -expand_cond_len_unop (rtx_code code, rtx *ops)
> +/* Subroutine to expand COND_LEN_* patterns.  */
> +static void
> +expand_cond_len_op (rtx_code code, unsigned icode, int op_num, rtx *cond_ops,
> +		    rtx len)
>  {

Would you mind renaming op_num (i.e. usually understood as operand_number) into
num_ops or nops? (i.e. number of operands).  That way we would be more in line of
what the later expander functions do.

> -  rtx dest = ops[0];
> -  rtx mask = ops[1];
> -  rtx src = ops[2];
> -  rtx merge = ops[3];
> -  rtx len = ops[4];
> +  rtx dest = cond_ops[0];
> +  rtx mask = cond_ops[1];

I would actually prefer to keep "ops" because it's already clear from the
function name that we work with a conditional function (and we don't have
any other ops).

>  
> +/* Expand unary ops COND_LEN_*.  */
> +void
> +expand_cond_len_unop (rtx_code code, rtx *ops)
> +{
> +  rtx dest = ops[0];
> +  rtx mask = ops[1];
> +  rtx src = ops[2];
> +  rtx merge = ops[3];
> +  rtx len = ops[4];
> +
> +  machine_mode mode = GET_MODE (dest);
> +  insn_code icode = code_for_pred (code, mode);
> +  rtx cond_ops[RVV_UNOP_MASK] = {dest, mask, merge, src};
> +  expand_cond_len_op (code, icode, RVV_UNOP_MASK, cond_ops, len);
> +}

We're already a bit inconsistent with how we pasds mask, merge and the source
operands.  Maybe we could also unify this a bit?  I don't have a clear
preference for either, though.

> +  rtx cond_ops[RVV_BINOP_MASK] = {dest, mask, merge, src1, src2};

Here, the merge comes before the sources as well.

> +  rtx cond_ops[RVV_TERNOP_MASK] = {dest, mask, src1, src2, src3, merge};
And here, the merge comes last.  I realize this makes sense in the context
of a ternary operation because the merge is always "real".  As our vector
patterns are similar, maybe we should use this ordering all the time?

Regards
 Robin

Re: [PATCH] RISC-V: Refactor and clean expand_cond_len_{unop,binop,ternop}

[Lehua Ding]

Hi Robin,

Thanks for reviewing.

> Cleanup up here is good, right now it's not really an insn_type but
> indeed just the number of operands.  My original idea was to have an
> insn type and a mostly unified expander that performs all necessary
> operations depending on the insn_type.  Just to give an idea of why it's
> called that way.
> 
>> +  rtx ops[RVV_BINOP_MASK] = {target, mask, target, op, sel};
>> +  emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MASK, ops);
> 
> One of the ideas was that a function emit_vlmax_masked_mu_insn would already
> know that it's dealing with a mask and we would just pass something like
> RVV_BINOP.  The other way would be to just have emit_vlmax_mu_insn or
> something and let the rest be deduced from the insn_type.  Even the vlmax
> I intended to have mostly implicit but that somehow got lost during
> refactorings :)  No need to change anything for now, just for perspective
> again.

I think the ideas of these two comments will be reflected in the next 
patch of refactoring emit_vlmax/nonvlmax_xxx functions (abstract several 
uniform types of RVV instructions), thanks a lot!


> Would you mind renaming op_num (i.e. usually understood as operand_number) into
> num_ops or nops? (i.e. number of operands).  That way we would be more in line of
> what the later expander functions do.

OK, rename op_num into num_ops.


> I would actually prefer to keep "ops" because it's already clear from the
> function name that we work with a conditional function (and we don't have
> any other ops).

No problem.


> We're already a bit inconsistent with how we pasds mask, merge and the source
> operands.  Maybe we could also unify this a bit?  I don't have a clear
> preference for either, though.
> 
>> +  rtx cond_ops[RVV_BINOP_MASK] = {dest, mask, merge, src1, src2};
> 
> Here, the merge comes before the sources as well.
> 
>> +  rtx cond_ops[RVV_TERNOP_MASK] = {dest, mask, src1, src2, src3, merge};
> And here, the merge comes last.  I realize this makes sense in the context
> of a ternary operation because the merge is always "real".  As our vector
> patterns are similar, maybe we should use this ordering all the time?

Yes, the ternary merge is not placed in operand 2 like binop or unop, I 
discussed it with Juzhe and it could be unified, but it would change to 
the intrinsic part, so I would suggest to bring up a separate patch to 
unfied the operand order. The unfied order like this:

   DEST, MASK, MERGE (this three operands fixed for most insns)
   OPS (the number can be 1, 2, 3)
   VL, TAIL_POLICY, MASK_POLICY, AVL_TYPE, ROUDING_MODE


-- 
Best,
Lehua

Re: [PATCH] RISC-V: Refactor and clean expand_cond_len_{unop,binop,ternop}

[Lehua Ding]

Here is the V3 patch fix the comments, thanks.

https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628650.html

-- 
Best,
Lehua