public inbox for
 help / color / mirror / Atom feed
From: Lehua Ding <>
To: Robin Dapp <>,
Subject: Re: [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions
Date: Thu, 31 Aug 2023 17:59:53 +0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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")
	    [(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)
	        (match_operand:<V_DOUBLE_TRUNC> 3 "register_operand"))
	        (match_operand:<V_DOUBLE_TRUNC> 4 "register_operand")))
	    (match_operand:VWEXTI 2 "register_operand"))
	  (match_dup 2)))]
   [(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 


      reply	other threads:[~2023-08-31 10:00 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-30 11:53 Lehua Ding
2023-08-31  7:51 ` [PATCH] RISC-V: Refactor and clean emit_{vlmax, nonvlmax}_xxx functions Kito Cheng
2023-08-31  8:59   ` Lehua Ding
2023-08-31  9:16 ` [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions Robin Dapp
2023-08-31  9:59   ` Lehua Ding [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).