On 9/23/20 7:56 PM, Taylor Simpson wrote: > > >>> On 8/31/20 4:10 PM, Taylor Simpson wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Richard Henderson <richard.hender...@linaro.org> >>>>> Sent: Monday, August 31, 2020 1:20 PM >>>>> 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 >>>>> >>> Ho hum. Maybe I'm trying to overthink this too much before tackling the >>> ultimate goal of full parsing of the SHORTCODE. >>> Perhaps the only thing for the short term is to have the generator grep >>> genptr.c for "#define fGEN", to choose between the two alternatives: >> inline >>> generation or out-of-line helper generation. >> >> That's certainly doable. It will also be good to implement some of your >> other >> ideas >> - Have the generator expand the DECL/READ/WRITE/FREE macros will make >> the generated code more readable and we can specialize them for >> predicated vs non-predicated instructions which will make translation faster. >> - Generate the entire generate_<tag> function instead of just the body will >> make the generated code more readable. > > 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. 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. 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. > I've also addressed several of the items from Richard's review, so I'll > resubmit the series once I figure out how to get "make check-tcg" working > under meson. Yes, make check-tcg is currently broken, as are a few other check-foo. I've not yet had the courage to look into it, hoping that someone else will do it first. r~