> -----Original Message-----
> From: ltaylorsimp...@gmail.com <ltaylorsimp...@gmail.com>
> Sent: Thursday, November 16, 2023 1:19 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: Thursday, November 16, 2023 10:25 AM
> > To: 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: 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
> > >
> > > > -----Original Message-----
> > > > From: Brian Cain <bc...@quicinc.com>
> > > > Sent: Wednesday, November 15, 2023 1:51 PM
> > > > To: Taylor Simpson <ltaylorsimp...@gmail.com>; qemu-
> > de...@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 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

Sorry, forgot it was in init_registers() context, I thought these were in 
global / __main__ context.

What about just returning the dictionary from init_registers()?

def init_registers():
    registers = (
        GprDest("R", "d"),
        GprDest("R", "e"),
        GprSource("R", "s"),
        GprSource("R", "t"),
        ...
        )
     registers = { reg.regtype + reg.regid: reg for reg in registers }

     return registers, new_registers

> 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

Reply via email to