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

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).

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

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.
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?

No need to address this right away though, just sharing some ideas again.


  parent reply	other threads:[~2023-08-31  9:16 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 ` Robin Dapp [this message]
2023-08-31  9:59   ` [PATCH] RISC-V: Refactor and clean emit_{vlmax,nonvlmax}_xxx functions Lehua Ding

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).