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