> -----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
>
> 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: Wednesday, November 15, 2023 1:51 PM
> > To: Taylor Simpson <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: 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 ModifierSource(Source):
> > > + def genptr_decl(self, f, tag, regno):
> > > + f.write(f" const int {self.regN} = insn->regno[{regno}];\n")
> > > + f.write(f" TCGv {self.regV} = hex_gpr[{self.regN} +
> > HEX_REG_M0];\n")
> > > + def idef_arg(self, declared):
> > > + declared.append(self.regV)
> > > + declared.append(self.regN)
> > > +
> >
> > IMO it's easier to reason about a function if it doesn't modify its inputs
> > and
> > instead it returns the transformed input. If idef_arg instead returned a
> > new
> > list or returned an iterable for the caller to catenate, it would be
> > clearer.
>
> We should figure out a better way to handle the special case of modifier
> registers. For every other register type,
> Idef_arg simply returns self.regV. For circular addressing, we also need the
> value of the corresponding CS register. Currently,
> we solve this by passing the register number so that idef-parser can get the
> value (i.e., hex_gpr[HEX_REG_CS0 + self.regN]).
>
> We could have idef-parser skip the circular addressing instructions (it
> already
> skips the bit-reverse instructions). That seems
> like a big hammer though. Any other thoughts?
>
>
> > > +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.
> > > +def init_registers():
> > > + registers["Rd"] = GprDest("R", "d")
> > > + registers["Re"] = GprDest("R", "e")
> > > + registers["Rs"] = GprSource("R", "s")
> > > + registers["Rt"] = GprSource("R", "t")
> > > + registers["Ru"] = GprSource("R", "u")
> > > + registers["Rv"] = GprSource("R", "v")
> > > + registers["Rx"] = GprReadWrite("R", "x")
> > > + registers["Ry"] = GprReadWrite("R", "y")
> > > + registers["Cd"] = ControlDest("C", "d")
> > > + registers["Cs"] = ControlSource("C", "s")
> > > + registers["Mu"] = ModifierSource("M", "u")
> > > + registers["Pd"] = PredDest("P", "d")
> > > + registers["Pe"] = PredDest("P", "e")
> > > + registers["Ps"] = PredSource("P", "s")
> > > + registers["Pt"] = PredSource("P", "t")
> > > + registers["Pu"] = PredSource("P", "u")
> > > + registers["Pv"] = PredSource("P", "v")
> > > + registers["Px"] = PredReadWrite("P", "x")
> > > + registers["Rdd"] = PairDest("R", "dd")
> > > + registers["Ree"] = PairDest("R", "ee")
> > > + registers["Rss"] = PairSource("R", "ss")
> > > + registers["Rtt"] = PairSource("R", "tt")
> > > + registers["Rxx"] = PairReadWrite("R", "xx")
> > > + registers["Ryy"] = PairReadWrite("R", "yy")
> > > + registers["Cdd"] = ControlPairDest("C", "dd")
> > > + registers["Css"] = ControlPairSource("C", "ss")
> > > + registers["Vd"] = VRegDest("V", "d")
> > > + registers["Vs"] = VRegSource("V", "s")
> > > + registers["Vu"] = VRegSource("V", "u")
> > > + registers["Vv"] = VRegSource("V", "v")
> > > + registers["Vw"] = VRegSource("V", "w")
> > > + registers["Vx"] = VRegReadWrite("V", "x")
> > > + registers["Vy"] = VRegTmp("V", "y")
> > > + registers["Vdd"] = VRegPairDest("V", "dd")
> > > + registers["Vuu"] = VRegPairSource("V", "uu")
> > > + registers["Vvv"] = VRegPairSource("V", "vv")
> > > + registers["Vxx"] = VRegPairReadWrite("V", "xx")
> > > + registers["Qd"] = QRegDest("Q", "d")
> > > + registers["Qe"] = QRegDest("Q", "e")
> > > + registers["Qs"] = QRegSource("Q", "s")
> > > + registers["Qt"] = QRegSource("Q", "t")
> > > + 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 }
>
> Will work on this.
>
> > In general this looks like a good change to me.
>
> Thanks for the feedback,
> Taylor
>
