Re: [RFC PATCH v3 30/34] Hexagon (target/hexagon) TCG for instructions with multiple definitions

[Richard Henderson]

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.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
>>>>>
>>> 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~