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

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Thursday, November 16, 2023 10:25 AM
> To: 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: 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
> >
> > > -----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 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