RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Wednesday, November 15, 2023 1:51 PM
> To: Taylor Simpson <ltaylorsimpson@gmail.com>; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; Sid
> Manning <sidneym@quicinc.com>; richard.henderson@linaro.org;
> philmd@linaro.org; ale@rev.ng; anjo@rev.ng
> Subject: RE: [RFC PATCH] Hexagon (target/hexagon) Make generators object
> oriented
> 
> 
> 
> > -----Original Message-----
> > From: Taylor Simpson <ltaylorsimpson@gmail.com>
> > Sent: Thursday, November 9, 2023 3:26 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> > <quic_mathbern@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> anjo@rev.ng;
> > ltaylorsimpson@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 <ltaylorsimpson@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