RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: ltaylorsimp...@gmail.com
> Sent: Tuesday, December 5, 2023 11:08 AM
> To: 'Brian Cain' ; qemu-devel@nongnu.org
> Cc: 'Matheus Bernardino (QUIC)' ; 'Sid
> Manning' ; 'Marco Liebel (QUIC)'
> ; 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
> > Sent: Monday, December 4, 2023 9:46 PM
> > To: Taylor Simpson ; qemu-
> de...@nongnu.org
> > Cc: Matheus Bernardino (QUIC) ; Sid
> Manning
> > ; Marco Liebel (QUIC)
> ;
> > 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
> > > Sent: Monday, December 4, 2023 7:53 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > > ; Sid Manning ;
> > Marco
> > > Liebel (QUIC) ;
> > > 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
> > > ---
> > > 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
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: Brian Cain
> Sent: Monday, December 4, 2023 9:46 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; 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
> > Sent: Monday, December 4, 2023 7:53 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > 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
> > ---
> > 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/tar
RE: [PATCH 3/9] Hexagon (target/hexagon) Make generators object oriented - gen_helper_protos
> -Original Message-
> From: Taylor Simpson
> Sent: Monday, December 4, 2023 7:53 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; 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
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> Signed-off-by: Taylor Simpson
> ---
> 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
> @@ -22,39 +22,6 @@
> import string
> import hex_common
>
> -##
> -## Helpers for gen_helper_prototype
> -##
> -def_helper_types = {
> -"N": "s32",
> -"O": "s32",
> -"P": "s32",
> -"M": "s32",
> -"C": "s32",
> -"R": "s32",
> -"V": "ptr",
> -"Q": "ptr",
> -}
> -
> -def_helper_types_pair = {
> -"R": "s64",
> -"C": "s64",
> -"S": "s64",
> -"G": "s64",
> -"V": "ptr",
> -"Q": "ptr",
> -}
> -
> -
> -def gen_def_helper_opn(f, tag, regtype, regid, i):
> -if hex_common.is_pair(regid):
> -f.write(f", {def_helper_types_pair[regtype]}")
> -elif hex_common.is_single(regid):
> -f.write(f", {def_helper_types[regtype]}")
> -else:
> -hex_common.bad_register(regtype, regid)
> -
> -
> ##
> ## 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?
>
> if numscalarresults > 1:
> -## The helper is bogus when there is more than one result
> -f.write(f"DEF_HELPER_1({tag}, void, env)\n")
> -else:
> -## Figure out how many arguments the helper will take
> -if numscalarresults == 0:
> -def_helper_size = len(regs) + len(imms) + numscalarreadwrite + 1
> -if hex_common.need_pkt_has_multi_cof(tag):
> -def_helper_size += 1
> -if hex_common.need_pkt_need_commit(tag):
> -def_helper_size += 1
> -if hex_common.need_part1(tag):
> -def_helper_size += 1
> -if hex_common.need_slot(tag):
> -def_helper_size += 1
> -if hex_common.need_PC(tag):
> -def_helper_size += 1
> -if hex_common.helper_needs_next_PC(tag):
> -def_helper_size += 1
> -if hex_common.need_condexec_reg(tag, regs):
> -def_helper_size += 1
> -f.write(f"DEF_HELPER_{def_helper_size}({tag}")
> -## The return type is void
> -f.write(", void")
> -else:
> -def_helper_size = len(regs) + len(imms) + numscalarreadwrite
> -if hex_common.need_pkt_has_multi_cof(tag):
> -def_helper_size += 1
> -if hex_common.need_pkt_need_commit(tag):
> -def_helper_size += 1
> -if hex_common.need_part1(tag):
> -def_helper_size += 1
> -if hex_common.need_slot(tag):
> -def_helper_size += 1
> -if hex_common.need_PC(tag):
> -def_helper_size += 1
> -if hex_common.need_condexec_reg(tag, regs):
> -def_helper_size += 1
> -if hex_common.helper_needs_next_PC(tag):
> -def_helper_size += 1
> -f.write(f"DEF_HELPER_{def_helper_size}({tag}")
> -
> -## Generate the qemu DEF_HELPER type for each result
> -## Iterate over this list twice
> -## - Emit the scalar result
> -## - Emit the vector result
> -i = 0
> -for regtype,
