From: <ltaylorsimpson@gmail.com>
To: "'Brian Cain'" <bcain@quicinc.com>, <qemu-devel@nongnu.org>
Cc: "'Matheus Bernardino \(QUIC\)'" <quic_mathbern@quicinc.com>,
"'Sid Manning'" <sidneym@quicinc.com>,
"'Marco Liebel \(QUIC\)'" <quic_mliebel@quicinc.com>,
<richard.henderson@linaro.org>, <philmd@linaro.org>, <ale@rev.ng>,
<anjo@rev.ng>
Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
Date: Tue, 5 Dec 2023 16:59:55 -0600 [thread overview]
Message-ID: <0f2601da27ce$c41556d0$4c400470$@gmail.com> (raw)
In-Reply-To: <0e5901da279d$a14e8f80$e3ebae80$@gmail.com>
> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Tuesday, December 5, 2023 11:08 AM
> To: 'Brian Cain' <bcain@quicinc.com>; qemu-devel@nongnu.org
> Cc: 'Matheus Bernardino (QUIC)' <quic_mathbern@quicinc.com>; 'Sid
> Manning' <sidneym@quicinc.com>; 'Marco Liebel (QUIC)'
> <quic_mliebel@quicinc.com>; richard.henderson@linaro.org;
> philmd@linaro.org; ale@rev.ng; anjo@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -----Original Message-----
> > From: Brian Cain <bcain@quicinc.com>
> > Sent: Monday, December 4, 2023 9:46 PM
> > To: Taylor Simpson <ltaylorsimpson@gmail.com>; qemu-
> devel@nongnu.org
> > Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; Sid
> Manning
> > <sidneym@quicinc.com>; Marco Liebel (QUIC)
> <quic_mliebel@quicinc.com>;
> > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> > anjo@rev.ng
> > Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators
> > object oriented - gen_helper_protos
> >
> >
> >
> > > -----Original Message-----
> > > From: Taylor Simpson <ltaylorsimpson@gmail.com>
> > > Sent: Monday, December 4, 2023 7:53 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> > > <quic_mathbern@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> > Marco
> > > Liebel (QUIC) <quic_mliebel@quicinc.com>;
> > > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> > > anjo@rev.ng; ltaylorsimpson@gmail.com
> > > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > > oriented - gen_helper_protos
> > >
> > > Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> > > ---
> > > target/hexagon/gen_helper_protos.py | 184 ++++++++--------------------
> > > target/hexagon/hex_common.py | 15 +--
> > > 2 files changed, 55 insertions(+), 144 deletions(-)
> > >
> > > diff --git a/target/hexagon/gen_helper_protos.py
> > > b/target/hexagon/gen_helper_protos.py
> > > index 131043795a..9277199e1d 100755
> > > --- a/target/hexagon/gen_helper_protos.py
> > > +++ b/target/hexagon/gen_helper_protos.py
> > > ##
> > > ## Generate the DEF_HELPER prototype for an instruction
> > > ## For A2_add: Rd32=add(Rs32,Rt32)
> > > @@ -65,116 +32,62 @@ def gen_helper_prototype(f, tag, tagregs,
> > tagimms):
> > > regs = tagregs[tag]
> > > imms = tagimms[tag]
> > >
> > > - numresults = 0
> > > + ## If there is a scalar result, it is the return type
> > > + return_type = ""
> >
> > Should we use `return_type = None` here?
> >
> > > numscalarresults = 0
> > > - numscalarreadwrite = 0
> > > for regtype, regid in regs:
> > > - if hex_common.is_written(regid):
> > > - numresults += 1
> > > - if hex_common.is_scalar_reg(regtype):
> > > + reg = hex_common.get_register(tag, regtype, regid)
> > > + if reg.is_written() and reg.is_scalar_reg():
> > > + return_type = reg.helper_proto_type()
> > > numscalarresults += 1
> > > - if hex_common.is_readwrite(regid):
> > > - if hex_common.is_scalar_reg(regtype):
> > > - numscalarreadwrite += 1
> > > + if numscalarresults == 0:
> > > + return_type = "void"
> >
> > Should we use `return_type = None` here?
>
> I don't see a point of setting it to None. By the time it gets to the use below,
> it will definitely have a value. We could initialize it to void instead of "" and
> skip this check.
>
>
> > > +
> > > + ## Other stuff the helper might need
> > > + if hex_common.need_pkt_has_multi_cof(tag):
> > > + declared.append("i32")
> > > + if hex_common.need_pkt_need_commit(tag):
> > > + declared.append("i32")
> > > + if hex_common.need_PC(tag):
> > > + declared.append("i32")
> > > + if hex_common.need_next_PC(tag):
> > > + declared.append("i32")
> > > + if hex_common.need_slot(tag):
> > > + declared.append("i32")
> > > + if hex_common.need_part1(tag):
> > > + declared.append("i32")
> >
> > What do you think of this instead? The delta below is on top of this patch.
> >
> > --- a/target/hexagon/gen_helper_protos.py
> > +++ b/target/hexagon/gen_helper_protos.py
> > @@ -73,18 +73,9 @@ def gen_helper_prototype(f, tag, tagregs, tagimms):
> > declared.append("s32")
> >
> > ## Other stuff the helper might need
> > - if hex_common.need_pkt_has_multi_cof(tag):
> > - declared.append("i32")
> > - if hex_common.need_pkt_need_commit(tag):
> > - declared.append("i32")
> > - if hex_common.need_PC(tag):
> > - declared.append("i32")
> > - if hex_common.need_next_PC(tag):
> > - declared.append("i32")
> > - if hex_common.need_slot(tag):
> > - declared.append("i32")
> > - if hex_common.need_part1(tag):
> > - declared.append("i32")
> > + for stuff in hex_common.other_stuff:
> > + if stuff(tag):
> > + declared.append('i32')
> >
> > arguments = ", ".join(declared)
> > f.write(f"DEF_HELPER_{len(declared) - 1}({tag}, {arguments})\n")
> > diff --git a/target/hexagon/gen_tcg_funcs.py
> > b/target/hexagon/gen_tcg_funcs.py index 8c2bc03c10..cb02d91886 100755
> > --- a/target/hexagon/gen_tcg_funcs.py
> > +++ b/target/hexagon/gen_tcg_funcs.py
> > @@ -109,18 +109,9 @@ def gen_tcg_func(f, tag, regs, imms):
> >
> >
> declared.append(f"tcg_constant_tl({hex_common.imm_name(immlett)})")
> >
> > ## Other stuff the helper might need
> > - if hex_common.need_pkt_has_multi_cof(tag):
> > - declared.append("tcg_constant_tl(ctx->pkt->pkt_has_multi_cof)")
> > - if hex_common.need_pkt_need_commit(tag):
> > - declared.append("tcg_constant_tl(ctx->need_commit)")
> > - if hex_common.need_PC(tag):
> > - declared.append("tcg_constant_tl(ctx->pkt->pc)")
> > - if hex_common.need_next_PC(tag):
> > - declared.append("tcg_constant_tl(ctx->next_PC)")
> > - if hex_common.need_slot(tag):
> > - declared.append("gen_slotval(ctx)")
> > - if hex_common.need_part1(tag):
> > - declared.append("tcg_constant_tl(insn->part1)")
> > + for stuff, text in hex_common.other_stuff:
> > + if stuff(tag):
> > + declared.append(text)
> >
> > arguments = ", ".join(declared)
> > f.write(f" gen_helper_{tag}({arguments});\n")
> > diff --git a/target/hexagon/hex_common.py
> > b/target/hexagon/hex_common.py index 90d61a1b16..954532921d 100755
> > --- a/target/hexagon/hex_common.py
> > +++ b/target/hexagon/hex_common.py
> > @@ -1028,3 +1028,13 @@ def get_register(tag, regtype, regid):
> > return registers[f"{regtype}{regid}"]
> > else:
> > return new_registers[f"{regtype}{regid}"]
> > +
> > +
> > +other_stuff = {
> > + need_pkt_has_multi_cof: "tcg_constant_tl(ctx->pkt-
> > >pkt_has_multi_cof)",
> > + need_pkt_need_commit: "tcg_constant_tl(ctx->need_commit)",
> > + need_PC: "tcg_constant_tl(ctx->pkt->pc)",
> > + need_next_PC: "tcg_constant_tl(ctx->next_PC)",
> > + need_slot: "gen_slotval(ctx)",
> > + need_part1: "tcg_constant_tl(insn->part1)", }
>
> Great idea to centralize these in hex_common. There are three parts
> though. There's the actual helper argument in gen_hepler_funcs.py. I'll
> think about how best to cover the 3 parts. Let me know if you have ideas as
> well.
>
> Thanks,
> Taylor
How about this?
In hex_common.py
class HelperArg:
def __init__(self, proto_arg, call_arg, func_arg):
self.proto_arg = proto_arg
self.call_arg = call_arg
self.func_arg = func_arg
def extra_helper_args(tag):
args = []
if need_pkt_has_multi_cof(tag):
args.append(HelperArg(
"i32",
"tcg_constant_tl(ctx->pkt->pkt_has_multi_cof)",
"uint32_t pkt_has_multi_cof"
))
if need_pkt_need_commit(tag):
args.append(HelperArg(
"i32",
"tcg_constant_tl(ctx->need_commit)",
"uint32_t pkt_need_commit"
))
...
return args
In gen_tcg_funcs.py
for extra_arg in hex_common.extra_helper_args(tag):
declared.append(extra_arg.call_arg)
In gen_helper_protos.py
for extra_arg in hex_common.extra_helper_args(tag):
declared.append(extra_arg.proto_arg)
In gen_helper_funcs.py
for extra_arg in hex_common.extra_helper_args(tag):
declared.append(extra_arg.func_arg)
A further refinement would be to have all the registers and immediates create a HelperArg. Then, we could consolidate the logic to create the argument lists in hex_common.py. For example, we pass the destination register as an input to the helper for predicated instructions, and the pointer to the destination for HVX.
Thanks,
Taylor
next prev parent reply other threads:[~2023-12-05 23:00 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-05 1:52 [PATCH 0/9] Hexagon (target/hexagon) Make generators object oriented Taylor Simpson
2023-12-05 1:52 ` [PATCH 1/9] Hexagon (target/hexagon) Clean up handling of modifier registers Taylor Simpson
2023-12-05 3:04 ` Brian Cain
2023-12-05 1:52 ` [PATCH 2/9] Hexagon (target/hexagon) Make generators object oriented - gen_tcg_funcs Taylor Simpson
2023-12-05 3:01 ` Brian Cain
2023-12-05 1:52 ` [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos Taylor Simpson
2023-12-05 3:46 ` Brian Cain
2023-12-05 17:08 ` ltaylorsimpson
2023-12-05 22:59 ` ltaylorsimpson [this message]
2023-12-05 1:52 ` [PATCH 4/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_funcs Taylor Simpson
2023-12-05 1:52 ` [PATCH 5/9] Hexagon (target/hexagon) Make generators object oriented - gen_idef_parser_funcs Taylor Simpson
2023-12-05 1:53 ` [PATCH 6/9] Hexagon (target/hexagon) Make generators object oriented - gen_op_regs Taylor Simpson
2023-12-05 3:02 ` Brian Cain
2023-12-05 1:53 ` [PATCH 7/9] Hexagon (target/hexagon) Make generators object oriented - gen_analyze_funcs Taylor Simpson
2023-12-05 1:53 ` [PATCH 8/9] Hexagon (target/hexagon) Remove unused WRITES_PRED_REG attribute Taylor Simpson
2023-12-05 1:53 ` [PATCH 9/9] Hexagon (target/hexagon) Remove dead functions from hex_common.py Taylor Simpson
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='0f2601da27ce$c41556d0$4c400470$@gmail.com' \
--to=ltaylorsimpson@gmail.com \
--cc=ale@rev.ng \
--cc=anjo@rev.ng \
--cc=bcain@quicinc.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=quic_mathbern@quicinc.com \
--cc=quic_mliebel@quicinc.com \
--cc=richard.henderson@linaro.org \
--cc=sidneym@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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 NNTP newsgroup(s).