> -----Original Message-----
> From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> Sent: Tuesday, December 5, 2023 11:08 AM
> To: 'Brian Cain' <bc...@quicinc.com>; qemu-devel@nongnu.org
> Cc: 'Matheus Bernardino (QUIC)' <quic_mathb...@quicinc.com>; 'Sid
> Manning' <sidn...@quicinc.com>; 'Marco Liebel (QUIC)'
> <quic_mlie...@quicinc.com>; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> oriented - gen_helper_protos
>
>
>
> > -----Original Message-----
> > From: Brian Cain <bc...@quicinc.com>
> > Sent: Monday, December 4, 2023 9:46 PM
> > To: Taylor Simpson <ltaylorsimp...@gmail.com>; qemu-
> de...@nongnu.org
> > Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid
> Manning
> > <sidn...@quicinc.com>; Marco Liebel (QUIC)
> <quic_mlie...@quicinc.com>;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng
> > Subject: RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators
> > object oriented - gen_helper_protos
> >
> >
> >
> > > -----Original Message-----
> > > From: Taylor Simpson <ltaylorsimp...@gmail.com>
> > > Sent: Monday, December 4, 2023 7:53 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Brian Cain <bc...@quicinc.com>; Matheus Bernardino (QUIC)
> > > <quic_mathb...@quicinc.com>; Sid Manning <sidn...@quicinc.com>;
> > Marco
> > > Liebel (QUIC) <quic_mlie...@quicinc.com>;
> > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > a...@rev.ng; ltaylorsimp...@gmail.com
> > > Subject: [PATCH 3/9] Hexagon (target/hexagon) Make generators object
> > > oriented - gen_helper_protos
> > >
> > > Signed-off-by: Taylor Simpson <ltaylorsimp...@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
