> -----Original Message----- > From: Richard Henderson <richard.hender...@linaro.org> > Sent: Thursday, September 24, 2020 9:04 AM > To: Taylor Simpson <tsimp...@quicinc.com>; qemu-devel@nongnu.org > Cc: phi...@redhat.com; laur...@vivier.eu; riku.voi...@iki.fi; > aleksandar.m.m...@gmail.com; a...@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 (a...@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