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