Re: [Qemu-devel] [PATCH 12/12 v9] target-tilegx: Generate tcg instructions to execute to 1st system call

[Peter Maydell]

On 21 April 2015 at 22:01, Chen Gang <address@hidden> wrote:
> On 4/11/15 05:28, Chen Gang wrote:
>> On 4/10/15 06:19, Peter Maydell wrote:
>>> On 27 March 2015 at 11:07, Chen Gang <address@hidden> wrote:
>>>> +}
>>>> +
>>>> +static void gen_cmpltui(struct DisasContext *dc,
>>>> +                        uint8_t rdst, uint8_t rsrc, int8_t imm8)
>>>> +{
>>>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpltui r%d, r%d, %d\n",
>>>> +                  rdst, rsrc, imm8);
>>>> +    tcg_gen_setcondi_i64(TCG_COND_LTU, dest_gr(dc, rdst), load_gr(dc, 
>>>> rsrc),
>>>> +                        (uint64_t)imm8);
>>>> +}
>>>> +
>>>> +static void gen_cmpeqi(struct DisasContext *dc,
>>>> +                       uint8_t rdst, uint8_t rsrc, int8_t imm8)
>>>> +{
>>>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpeqi r%d, r%d, %d\n", rdst, rsrc, 
>>>> imm8);
>>>> +    tcg_gen_setcondi_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, 
>>>> rsrc),
>>>> +                        (uint64_t)imm8);
>>>> +}

> Oh, after check again, for me, the original implementation is OK:
>
>  - We need print disassembly code for tracing, and all related functions
>    are meaningful and match the whole function naming way in this file.
>
>  - All related functions are too simple to simplified (only 2 lines each).

But you don't need all these functions! Something like:

static void gen_cmpi(struct DisasContext *dc, TCGCond cond,
                    uint8_t rdst, uint8_t rsrc, int8_t imm8)
{
    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmp%si r%d, r%d, %d\n",
                  condname[cond], dest_gr(dc, rdst), load_gr(dc, rsrc),
                  imm8);
    tcg_gen_setcondi_i64(cond, dest_gr(dc, rdst), load_gr(dc, rsrc),
                         (uint64_t)imm8);
}

will work in place of both of the above (and does this CPU
really only have two kinds of compare-immediate? Some of the
case labels suggest not, so it would be better to just implement
all the compare-immediates together in one patch.)

You can probably use a function to do the sub-opcode-to-TCGCond
lookup too.

Having dozens of two line functions that all look incredibly
similar is a really strong sign that you haven't taken
advantage of the commonality between them. CPU instruction
sets are usually pretty regular if they're well designed and
the resulting translate.c should also look pretty regular.

thanks
-- PMM