> -----Original Message-----
> From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> Sent: Thursday, November 16, 2023 1:19 PM
> To: Brian Cain <bc...@quicinc.com>; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid Manning
> <sidn...@quicinc.com>; richard.hender...@linaro.org; phi...@linaro.org;
> a...@rev.ng; a...@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> > -----Original Message-----
> > From: Brian Cain <bc...@quicinc.com>
> > Sent: Thursday, November 16, 2023 10:25 AM
> > To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org
> > Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid
> > Manning <sidn...@quicinc.com>; richard.hender...@linaro.org;
> > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> > oriented
> >
> >
> >
> > > -----Original Message-----
> > > From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> > > Sent: Wednesday, November 15, 2023 4:03 PM
> > > To: Brian Cain <bc...@quicinc.com>; qemu-devel@nongnu.org
> > > Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid
> > Manning
> > > <sidn...@quicinc.com>; richard.hender...@linaro.org;
> > > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > object oriented
> > >
> > > > -----Original Message-----
> > > > From: Brian Cain <bc...@quicinc.com>
> > > > Sent: Wednesday, November 15, 2023 1:51 PM
> > > > To: Taylor Simpson <ltaylorsimp...@gmail.com>; qemu-
> > de...@nongnu.org
> > > > Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid
> > > > Manning <sidn...@quicinc.com>; richard.hender...@linaro.org;
> > > > phi...@linaro.org; a...@rev.ng; a...@rev.ng
> > > > Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > > object oriented
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Taylor Simpson <ltaylorsimp...@gmail.com>
> > > > > Sent: Thursday, November 9, 2023 3:26 PM
> > > > > To: qemu-devel@nongnu.org
> > > > > Cc: Brian Cain <bc...@quicinc.com>; Matheus Bernardino (QUIC)
> > > > > <quic_mathb...@quicinc.com>; Sid Manning <sidn...@quicinc.com>;
> > > > > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > > > a...@rev.ng;
> > > > > ltaylorsimp...@gmail.com
> > > > > Subject: [RFC PATCH] Hexagon (target/hexagon) Make generators
> > > > > object oriented
> > > > >
> > > > > RFC - This patch handles gen_tcg_funcs.py. I'd like to get
> > > > > comments on the general approach before working on the other
> > Python scripts.
> > > > >
> > > > > The generators are generally a bunch of Python if-then-else
> > > > > statements based on the regtype and regid. Encapsulate
> > > > > regtype/regid into a class hierarchy. Clients lookup the register
> > > > > and invoke methods.
> > > > >
> > > > > This has several advantages for making the code easier to read,
> > > > > understand, and maintain
> > > > > - The class name makes it more clear what the operand does
> > > > > - All the methods for a given type of operand are together
> > > > > - Don't need as many calls to hex_common.bad_register
> > > > > - We can remove the functions in hex_common that use regtype/regid
> > > > > (e.g., is_read)
> > > > >
> > > > > Signed-off-by: Taylor Simpson <ltaylorsimp...@gmail.com>
> > > > > ---
> > > > > diff --git a/target/hexagon/hex_common.py
> > > > b/target/hexagon/hex_common.py
> > > > > index 0da65d6dd6..13ee55b6b2 100755
> > > > > --- a/target/hexagon/hex_common.py
> > > > > +++ b/target/hexagon/hex_common.py
> > > > > +class PredReadWrite(ReadWrite):
> > > > > + def genptr_decl(self, f, tag, regno):
> > > > > + f.write(f" const int {self.regN} =
> > > > > insn->regno[{regno}];\n")
> > > > > + f.write(f" TCGv {self.regV} = tcg_temp_new();\n")
> > > > > + f.write(f" tcg_gen_mov_tl({self.regV},
> > hex_pred[{self.regN}]);\n")
> > > >
> > > > Instead of successive calls to f.write(), each passing their own
> > > > string with a newline, use triple quotes:
> > > >
> > > > f.write(f""" const int {self.regN} = insn->regno[{regno}];
> > > > TCGv {self.regV} = tcg_temp_new();
> > > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
> > > >
> > > > If necessary/appropriate, you can also use textwrap.dedent() to make
> > > > the leading whitespace look appropriate:
> > > > https://docs.python.org/3/library/textwrap.html#textwrap.dedent
> > > >
> > > > import textwrap
> > > > ...
> > > > f.write(textwrap.dedent(f""" const int {self.regN} = insn-
> > >regno[{regno}];
> > > > TCGv {self.regV} = tcg_temp_new();
> > > > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n"""))
> > > >
> > >
> > > The indenting is for the readability of the output. We could dedent
> > > everything and run the result through the indent utility as
> > > idef-parser does. Not sure it's worth it though.
> >
> > Regardless of textwrap.dedent(), I think the output is most readable with
> > triple-quotes. It's more readable than it is with multiple f.write("this
> > line\n")
> > invocations.
> >
> > The intent of calling textwrap.dedent is to allow you to not out-dent the
> > text
> > in the python program. Since the indentation of the other lines next to the
> > string literal are significant to the program, it might be odd/confusing to
> > see
> > the python program have out-dented text lines.
> >
> > Consider the readability of the python program:
> >
> > if foo:
> > f.write(textwrap.dedent(f"""\
> > const int {self.regN} = insn->regno[{regno}];
> > TCGv {self.regV} = tcg_temp_new();
> > tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);
> > """))
> > if bar:
> > f.write(textwrap.dedent(f"""\
> > int x = {bar.reg};
> > """)
> > vs
> >
> > if foo:
> > f.write(f"""\
> > const int {self.regN} = insn->regno[{regno}]; TCGv {self.regV} =
> > tcg_temp_new(); tcg_gen_mov_tl({self.regV}, hex_pred[{self.regN}]);\n""")
> > if bar:
> > f.write(f"""\
> > int x = {bar.reg};
> > """)
> >
> > The latter omits textwrap.dedent() - if we used the leading whitespace here
> > like we do in the former, the output text would have inconsistent
> > formatting.
>
> Let's go with the first version but add an indent. I'll use a little helper
> function:
> def code_fmt(txt):
> return textwrap.indent(textwrap.dedent(txt), " ")
>
> Then, the Python code would look like this:
> class PairSource(Register, Pair, OldSource):
> def decl_tcg(self, f, tag, regno):
> self.decl_reg_num(f, regno)
> f.write(code_fmt(f"""\
> TCGv_i64 {self.reg_tcg()} = tcg_temp_new_i64();
> tcg_gen_concat_i32_i64({self.reg_tcg()},
> hex_gpr[{self.reg_num}],
> hex_gpr[{self.reg_num} + 1]);
> """))
>
> The generated code will be as desired:
> static void generate_A2_vaddw(DisasContext *ctx)
> {
> Insn *insn G_GNUC_UNUSED = ctx->insn;
> const int RddN = insn->regno[0];
> TCGv_i64 RddV = get_result_gpr_pair(ctx, RddN);
> const int RssN = insn->regno[1];
> TCGv_i64 RssV = tcg_temp_new_i64();
> tcg_gen_concat_i32_i64(RssV,
> hex_gpr[RssN],
> hex_gpr[RssN + 1]);
> const int RttN = insn->regno[2];
> TCGv_i64 RttV = tcg_temp_new_i64();
> tcg_gen_concat_i32_i64(RttV,
> hex_gpr[RttN],
> hex_gpr[RttN + 1]);
> emit_A2_vaddw(ctx, ctx->insn, ctx->pkt, RddV, RssV, RttV);
> gen_log_reg_write_pair(ctx, RddN, RddV);
> }
>
> > > > > +def init_registers():
> > > > > + registers["Rd"] = GprDest("R", "d")
> > > > > + registers["Re"] = GprDest("R", "e")
> > > > > + registers["Qu"] = QRegSource("Q", "u")
> > > > > + registers["Qv"] = QRegSource("Q", "v")
> > > > > + registers["Qx"] = QRegReadWrite("Q", "x")
> > > > > +
> > > > > + new_registers["Ns"] = GprNewSource("N", "s")
> > > > > + new_registers["Nt"] = GprNewSource("N", "t")
> > > > > + new_registers["Pt"] = PredNewSource("P", "t")
> > > > > + new_registers["Pu"] = PredNewSource("P", "u")
> > > > > + new_registers["Pv"] = PredNewSource("P", "v")
> > > > > + new_registers["Os"] = VRegNewSource("O", "s")
> > > >
> > > > AFAICT the keys for registers and new_registers can be derived from
> > > > the values themselves. Rather than worry about copy/paste errors
> > > > causing these not to correspond, you can create a dictionary from an
> > iterable like so:
> > > >
> > > > registers = (
> > > > GprDest("R", "d"),
> > > > GprDest("R", "e"),
> > > > GprSource("R", "s"),
> > > > GprSource("R", "t"),
> > > > ...
> > > > )
> > > > registers = { reg.regtype + reg.regid for reg in registers }
>
> I couldn't get this to work exactly as you suggest. Perhaps it is my neophyte
> Python skills, but assigning to registers creates a variable local to the
> function
Sorry, forgot it was in init_registers() context, I thought these were in
global / __main__ context.
What about just returning the dictionary from init_registers()?
def init_registers():
registers = (
GprDest("R", "d"),
GprDest("R", "e"),
GprSource("R", "s"),
GprSource("R", "t"),
...
)
registers = { reg.regtype + reg.regid: reg for reg in registers }
return registers, new_registers
> rather than updating the global variable. So, I ended up with this:
> ef init_registers():
> regs = {
> GprDest("R", "d"),
> GprDest("R", "e"),
> GprSource("R", "s"),
> ...
> }
> for reg in regs:
> registers[f"{reg.regtype}{reg.regid}"] = reg
>
> new_regs = {
> GprNewSource("N", "s"),
> GprNewSource("N", "t"),
> ...
> }
> for reg in new_regs:
> new_registers[f"{reg.regtype}{reg.regid}"] = reg
>
>
> Thanks,
> Taylor
