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

[Chen Gang]

On 4/22/15 05:15, Peter Maydell wrote:
> 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.
> 

I guess what you said is correct, but at present, I did not think of all
gen_cmp* (but it should really be done at last).

So for me, at present, we can leave it as current implementation (Add
FIXME comment). And at last (when almost finish all opcode decoding), I
shall rewrite it again.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed