Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions

[Lehua Ding <lehua.ding@rivai.ai>]

Hi Robin,

Thanks for these comments.

On 2023/8/31 17:16, Robin Dapp wrote:
> Hi Lehua,
> 
> thanks, this definitely goes into the direction of what I had in mind and
> simplifies a lot of the reduntant emit_... so it's good to have it.
> 
> I was too slow for a detailed response :)  So just some high-level comments.
> 
> One thing I noticed is the overloading of "MASK_OP",  we use it as
> "operation on masks" i.e. an insn as well as "mask policy".  IMHO we could
> get rid of UNARY_MASK_OP and BINARY_MASK_OP and just decide whether to
> add a mask policy depending on if all operands are masks (the same way we
> did before).

I think I should change the marco name here form __MASK_OP to 
__NORMAL_OP_MASK. MASK_OP means this is a insn operate on mask operands. 
I lift up it here because I want to simplify the emit_insn method.

> Related, and seeing that the MASK in UNARY_MASK_OP is somewhat redundant,
> I feel we still mix concerns a bit.  For example it is not obvious, from
> the name at least, why a WIDEN_TERNARY_OP does not have a merge operand
> and the decision making seems very "enum centered" now :D

As the above says, UNARY_MASK_OP means it is an operator of mask 
operands and itself don't have mask operand. If the operation depends on 
a mask, then the name should be UNARY_MASK_OP_TAMA.

For the WIDEN_TERNARY_OP, This is because of the design problem of 
vwmacc pattern (as bellow), maybe it can be unified with wmacc and then 
WIDEN_TERNARY_OP is not needed.

(define_insn "@pred_widen_mul_plus<su><mode>"
   [(set (match_operand:VWEXTI 0 "register_operand")
	(if_then_else:VWEXTI
	  (unspec:<VM>
	    [(match_operand:<VM> 1 "vector_mask_operand")
	     (match_operand 5 "vector_length_operand")
	     (match_operand 6 "const_int_operand")
	     (match_operand 7 "const_int_operand")
	     (match_operand 8 "const_int_operand")
	     (reg:SI VL_REGNUM)
	     (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
	  (plus:VWEXTI
	    (mult:VWEXTI
	      (any_extend:VWEXTI
	        (match_operand:<V_DOUBLE_TRUNC> 3 "register_operand"))
	      (any_extend:VWEXTI
	        (match_operand:<V_DOUBLE_TRUNC> 4 "register_operand")))
	    (match_operand:VWEXTI 2 "register_operand"))
	  (match_dup 2)))]
   "TARGET_VECTOR"
   "vwmacc<u>.vv\t%0,%3,%4%p1"
   [(set_attr "type" "viwmuladd")
    (set_attr "mode" "<V_DOUBLE_TRUNC>")])


> In general we use the NULLARY, UNARY, BINARY, TERNARY prefixes just
> to determine the number of sources which doesn't seem really necessary
> because a user of e.g. NEG will already know that there only is one
> source - he already specified it and currently needs to, redundantly,
> say UNARY again.

 From the caller's point of view, he does know how many sources, but the 
emit_insn method needs to use this flag(NULLARY_OP_P, UNARY_OP_P) to 
determine how many sources to add.

Oh, you mean we can get oerands number from insn_data[icode].n_operands? 
In this case, we don't really need to distinguish between NULLARY, UNARY 
and ect.

> If we split off the destination and sources from mask, merge and the rest
> we could ditch them altogether.
> 
> What about
>   emit_(non)vlmax_insn (icode, *operands (just dest and sources),
> 		       mask, merge, tail/mask policy, frm)
> 
> with mask defaulting to NULL and merge defaulting to VUNDEF?  So ideally,
> and in the easy case, the call would just degenerate to
>   emit_..._insn (icode, operands).
>   
> I realize this will cause some complications on the "other side" but with
> the enum in place it should still be doable?

That is to say, when the user needs to emit a COND_NEG, do you need to 
call it like this?:
   emit_vlmax_insn (pred_neg, ops, mask, vundef(), ta, ma)

Is there too much focus on operands that don't need to be passed in? 
Like merge, tail and mask policy operands.

Maybe I don't understand enough here, can you explain it in some more 
detail?

-- 
Best,
Lehua