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: Monday, August 31, 2020 1:20 PM
> 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
>
> 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~