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

[Brian Cain]

> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Wednesday, November 15, 2023 4:03 PM
> To: Brian Cain <bcain@quicinc.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
> 
> 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 <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.

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
>