[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions w
From: |
Taylor Simpson |
Subject: |
RE: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions with multiple definitions |
Date: |
Thu, 24 Sep 2020 17:18:13 +0000 |
> -----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