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 11:29 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
>
> On 8/31/20 10:08 AM, Taylor Simpson wrote:
> > There are some assumptions here I'd like to clarify.  When I started this
> > discussion, there seemed to be general resistance to the concept of a
> > generator.  Because of this, I made the generator as simple as possible and
> > pushed the complexity and optimization into code that consumes the
> output of
> > the generator.  Also, I assumed people wouldn't read the output of the
> > generator, so I didn't focus on making it readable.
>
> Except, at some point, one has to debug this code.
> If the code is unreadable, how do you figure out what's broken?
>
> > Now, it sounds like my assumptions were not correct.  You pointed out two
> > things to do in the generator> - Expand the macros inline
> > - Skip the instructions that have overrides
>
> Yes please.
>
> > I addition, there other things I should do if we want the generated files to
> be more readable
> > - Indent the code
>
> Helpful, yes.
>
> I wouldn't worry about nested statements within the *.def files too much,
> except to put each ";" terminated statement on a new line, so that gdb's step
> command goes to the next statement instead of skipping everything all at
> once.
>
> > - Only generate one of the fGEN_TCG_<tag> or gen_helper_<tag>
>
> That would be part of "skip the instructions that have overrides", would it
> not?

Just to be explicit, the code that generates code is generated as
    #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);
    #endif
If we're going to have the generator know if there is an override, this would 
be more readable as either
    fGEN_TCG_A2_add({ RdV=RsV+RtV;});
or
    gen_helper_A2_add(RdV, cpu_env, RsV, RtV);


> > - Generate the declaration of the generate_<tag> function instead of just
> the body
>
> I'm not quite sure what this means.
>
> Aren't the "generate_<tag>" functions private to genptr.c?  Why would we
> need a
> separate declaration of them, as opposed to just a definition?

In genptr.c, there is
    #define DEF_TCG_FUNC(TAG, GENFN) \
    static void generate_##TAG(CPUHexagonState *env, DisasContext *ctx, \
                               insn_t *insn, packet_t *pkt) \
    { \
        GENFN \
    }
    #include "tcg_funcs_generated.h"
    #undef DEF_TCG_FUNC
The generated code only has the body of the function.  It would be more 
readable to move the static void generate_##TAG ... into the generated code.