> -----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 > > The fGEN_TCG_A2_add macro does not require nor use that {...} argument.
The fGEN_TCG_A2_add macro does need that argument, but there are cases that do need it. Here's an example from gen_tcg.h #define fGEN_TCG_L2_loadrub_pr(SHORTCODE) SHORTCODE This is explained in the README, but basically the argument is useful if we can properly define the macros that it contains to generate TCG. > What it *does* need are the same arguments as are given to generate_<rtag>. I > assume you are using those arguments implicitly in your current > fGEN_TCG_<rtag> > instances? Yes > > It would be cleanest to only have the generate_* functions. > > Either they are written by hand (replacing the current fGEN_TCG_*), or they > are > generated. In either case, there's just the one level of indirection from > opcode_genptr[]. > > I'd imagine > > --- genptr.c > > #define DEF_TCG_FUNC(TAG) \ > static void generate_##TAG(CPUHexagonState *env, \ > DisasContext *ctx, insn_t *insn, packet_t *pkt) > > /* > * All IIDs with an explicit implementation, > * overriding the auto-generated helper functions. > */ > > DEF_TCG_FUNC(A2_add) > { > /* { RdV=RsV+RtV;} */ > tcg_gen_add_tl(args...); > } There's additional generated code before and after the tcg_gen_add_tl. IMO, we don't want the person who writes an override having to reproduce the generated code. Assuming we have a definition of fGEN_TCG_A2_add and we have the generator intelligently expanding the macros, this is what will be generated. static void generate_A2_add(CPUHexagonState *env, DisasContext *ctx, insn_t *insn, packet_t *pkt) { /* A2_add */ int RdN =insn->regno[0]; TCGv RdV = tcg_temp_local_new(); int RsN = insn->regno[1]; TCGv RsV = hex_gpr[RsN]; int RtN = insn->regno[2]; TCGv RtV = hex_gpr[RtN]; fGEN_TCG_A2_add({ RdV=RsV+RtV;}); gen_log_reg_write(RdN, RdV, insn->slot, 0); ctx_log_reg_write(ctx, RdN); tcg_temp_free(RdV); /* A2_add */ } If there weren't an override, we'd get this static void generate_A2_add(CPUHexagonState *env, DisasContext *ctx, insn_t *insn, packet_t *pkt) { /* A2_add */ int RdN =insn->regno[0]; TCGv RdV = tcg_temp_local_new(); int RsN = insn->regno[1]; TCGv RsV = hex_gpr[RsN]; int RtN = insn->regno[2]; TCGv RtV = hex_gpr[RtN]; gen_helper_A2_add(RdV, cpu_env, RsV, RtV); /* Only difference is this line */ gen_log_reg_write(RdN, RdV, insn->slot, 0); ctx_log_reg_write(ctx, RdN); tcg_temp_free(RdV); /* A2_add */ } The fGEN_TCG_<tag> macro can also mention the operands of the instruction (RdV, RsV, RtV in this example). Unlike the generate_<tag> functions that all have the same signature. The overrides would have different signatures. This would be more defensive programming because you know exactly where the variables come from but more verbose when writing the overrides by hand. Also, note that these need to be macros in order to take advantage of the SHORTCODE. In other words, instead of #define fGEN_TCG_A2_add(SHORTCODE) tcg_gen_add_tl(RdV, RsV, RtV) We would write #define fGEN_TCG_A2_add(env, ctx, insn, pkt, RdV, RsV, RtV, SHORTCODE) tcg_gen_add_tl(RdV, RsV, RtV); Personally, I prefer the former, but will change to the latter if you feel strongly. I'm not married to the fGEN_TCG_<tag> name. DEF_TCG_<tag> would also be fine. > > /* > * Generate calls to the auto-generate helpers, > * and slot everything into the opcode_genptr table. > */ > #include "genptr_generated.c.inc" > > --- genptr_generated.c.inc > > DEF_TCG_FUNC(A4_tlbmatch) > { > gen_helper_A4_tlbmatch(args...); > } > > // etc > > const SemanticInsn opcode_genptr[] = { > // All IID's, generated or not. > }; > > --- > > This leaves genptr.c as the file to grep for '^DEF_TCG_FUNC'. > > > r~