RE: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions with multiple definitions

[Taylor Simpson]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, September 24, 2020 9:04 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for
> instructions with multiple definitions
>
> >
> > I've made these changes to the generator.  I hope you like the results.  As
> an example, here is what we generate for the add instruction
> >
> > DEF_TCG_FUNC(A2_add,
> > static void generate_A2_add(
> >                 CPUHexagonState *env,
> >                 DisasContext *ctx,
> >                 insn_t *insn,
> >                 packet_t *pkt)
> > {
> >     TCGv RdV = tcg_temp_local_new();
> >     const int RdN = insn->regno[0];
> >     TCGv RsV = hex_gpr[insn->regno[1]];
> >     TCGv RtV = hex_gpr[insn->regno[2]];
> >     gen_helper_A2_add(RdV, cpu_env, RsV, RtV);
> >     gen_log_reg_write(RdN, RdV);
> >     ctx_log_reg_write(ctx, RdN);
> >     tcg_temp_free(RdV);
> > })
>
> I would be happier if the entire function body were not in a macro.  Have you
> tried to set a gdb breakpoint in one of these?  Once upon a time at least, 
> this
> would have resulted in all lines of the function becoming one "source line" in
> the debug info.

Good point.  It still comes out as a single line.

> I also think the full function prototype is unnecessary, and the replication 
> of
> "A2_add" undesirable.
>
> I would prefer the function prototype itself to be macro-ized.
>
> E.g.
>
> DEF_TCG_FUNC(A2_add)
> {
>     TCGv RdV = tcg_temp_local_new();
>     const int RdN = insn->regno[0];
>     TCGv RsV = hex_gpr[insn->regno[1]];
>     TCGv RtV = hex_gpr[insn->regno[2]];
>     gen_helper_A2_add(RdV, cpu_env, RsV, RtV);
>     gen_log_reg_write(RdN, RdV);
>     ctx_log_reg_write(ctx, RdN);
>     tcg_temp_free(RdV);
> }
>
> with
>
> #define DEF_TCG_FUNC(TAG)                             \
>     static void generate_##TAG(CPUHexagonState *env,  \
>                                DisasContext *ctx,     \
>                                insn_t *insn,          \
>                                packet_t *pkt)
>
> > And here is how the generated file gets used in genptr.c
> >
> > #define DEF_TCG_FUNC(TAG, GENFN) \
> >     GENFN
> > #include "tcg_funcs_generated.h"
> > #undef DEF_TCG_FUNC
> >
> > /*
> >  * Not all opcodes have generate_<tag> functions, so initialize
> >  * the table from the tcg_funcs_generated.h file.
> >  */
> > const semantic_insn_t opcode_genptr[XX_LAST_OPCODE] = {
> > #define DEF_TCG_FUNC(TAG, GENFN) \
> >     [TAG] = generate_##TAG,
> > #include "tcg_funcs_generated.h"
> > #undef DEF_TCG_FUNC
> > };
>
> Obviously, the macro I propose above cannot be directly reused, as you do
> here.
>  But I also think we should not try to do so.
>
> You've got a script generating stuff.  It can just as easily generate two
> different lists.  You're trying to do too much with the C preprocessor and too
> little with python.

Sure, generating two lists was also suggested by Alessandro (ale@rev.ng).  
Although doing more in python and less with the C preprocessor would lead to 
the conclusion we shouldn't define the function prototype in a macro.  If we 
generate two lists, what is the advantage of putting the function signature in 
a macro vs generating?

>
> At some point in the v3 thread, I had suggested grepping for some macro in
> order to indicate to the python script which tags are implemented manually.
> My
> definition above is easy to look for: exactly one thing on the line, easy
> regexp.

This is already done as well.  As you may recall, we were previously generating
    #ifdef fGEN_TCG_A2_add
    fGEN_TCG_A2_add({ RdV=RsV+RtV;});
    #else
    do {
    gen_helper_A2_add(RdV, cpu_env, RsV, RtV);
    } while (0);

The generator now searches for "#define fGEN_TCG_<tag>" and generates different 
code depending on what it finds.  This version of the series only contains 
overrides that are required for correct execution.  So, A2_add isn't there.  
When we do override it, the generator produces this

static void generate_A2_add(
                CPUHexagonState *env,
                DisasContext *ctx,
                insn_t *insn,
                packet_t *pkt)
{
    TCGv RdV = tcg_temp_local_new();
    const int RdN = insn->regno[0];
    TCGv RsV = hex_gpr[insn->regno[1]];
    TCGv RtV = hex_gpr[insn->regno[2]];
    fGEN_TCG_A2_add({ RdV=RsV+RtV;});                  <---- This line is 
different
    gen_log_reg_write(RdN, RdV);
    ctx_log_reg_write(ctx, RdN);
    tcg_temp_free(RdV);
}

Also, if it finds the override, it doesn't generate the DEF_HELPER or the 
helper function.  That means we don't include tcg_gen.h in helper.h as you 
suggested.


Thanks,
Taylor