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

[Chen Gang]

On 4/10/15 06:19, Peter Maydell wrote:
> On 27 March 2015 at 11:07, Chen Gang <address@hidden> wrote:

[...]
>>
>> +static void gen_fnop(void)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n");
> 
> I really don't much like mixing a fake disassembler in
> with the translator. If you want a disassembler, write one
> and put it in disas/...
> 

For me, it is necessary (for I am not quite sure whether my disassemble
must be correct). It is only for tracing.

>> +}
>> +
>> +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);
>> +}
>> +
>> +static void gen_cmpne(struct DisasContext *dc,
>> +                      uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmpne r%d, r%d, r%d\n",
>> +                  rdst, rsrc, rsrcb);
>> +    tcg_gen_setcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc),
>> +                        load_gr(dc, rsrcb));
>> +}
>> +
>> +static void gen_cmoveqz(struct DisasContext *dc,
>> +                        uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmoveqz r%d, r%d, r%d\n",
>> +                  rdst, rsrc, rsrcb);
>> +    tcg_gen_movcond_i64(TCG_COND_EQ, dest_gr(dc, rdst), load_gr(dc, rsrc),
>> +                        load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, 
>> rdst));
>> +}
>> +
>> +static void gen_cmovnez(struct DisasContext *dc,
>> +                        uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "cmovnez r%d, r%d, r%d\n",
>> +                  rdst, rsrc, rsrcb);
>> +    tcg_gen_movcond_i64(TCG_COND_NE, dest_gr(dc, rdst), load_gr(dc, rsrc),
>> +                        load_zero(dc), load_gr(dc, rsrcb), load_gr(dc, 
>> rdst));
>> +}
> 
> This is hugely repetitive. Write a common function that takes a
> TCG_COND_* as a parameter.
> 

OK, thanks.

>> +
>> +static void gen_add(struct DisasContext *dc,
>> +                    uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "add r%d, r%d, r%d\n",
>> +                  rdst, rsrc, rsrcb);
>> +    tcg_gen_add_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), load_gr(dc, 
>> rsrcb));
>> +}
>> +
>> +static void gen_addimm(struct DisasContext *dc,
>> +                       uint8_t rdst, uint8_t rsrc, int64_t imm)
>> +{
>> +    tcg_gen_addi_i64(dest_gr(dc, rdst), load_gr(dc, rsrc), imm);
>> +}
>> +
>> +static void gen_addi(struct DisasContext *dc,
>> +                     uint8_t rdst, uint8_t rsrc, int8_t imm8)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "addi r%d, r%d, %d\n", rdst, rsrc, 
>> imm8);
>> +    gen_addimm(dc, rdst, rsrc, (int64_t)imm8);
> 
> This cast is pointless, because the 'imm' argument to
> gen_addimm() already has type int64_t.
> 

OK, thanks.

>> +}
>> +
>> +static void gen_addli(struct DisasContext *dc,
>> +                      uint8_t rdst, uint8_t rsrc, int16_t im16)
>> +{
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "addli r%d, r%d, %d\n", rdst, rsrc, 
>> im16);
>> +    gen_addimm(dc, rdst, rsrc, (int64_t)im16);
> 
> Another pointless cast.
> 

OK, thanks.

[...]
>> +
>> +/*
>> + * The related functional description for bfextu in isa document:
>> + *
>> + * uint64_t mask = 0;
>> + * mask = (-1ULL) ^ ((-1ULL << ((BFEnd - BFStart) & 63)) << 1);
>> + * uint64_t rot_src = (((uint64_t) rf[SrcA]) >> BFStart)
>> + *                    | (rf[SrcA] << (64 - BFStart));
>> + * rf[Dest] = rot_src & mask;
>> + */
>> +static void gen_bfextu(struct DisasContext *dc, uint8_t rdst, uint8_t rsrc,
>> +                       int8_t start, int8_t end)
>> +{
>> +    uint64_t mask = (-1ULL) ^ ((-1ULL << ((end - start) & 63)) << 1);
>> +    TCGv tmp = dest_gr(dc, rdst);
> 
> Are the start and end immediates here limited such that we're
> guaranteed not to hit any of C's undefined behaviour for out
> of range shifts, and that we don't hit TCG's undefined-value
> behaviour on bad rotates?
> 

For me, it is correct, it is only the copy of the document, which has
already considered about any cases (include C's undefined behaviour).

>>  static void decode_addi_opcode_y0(struct DisasContext *dc,
>>                                    tilegx_bundle_bits bundle)
>>  {
>> +    uint8_t rsrc = (uint8_t)get_SrcA_Y0(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_Y0(bundle);
>> +    int8_t imm8 = (int8_t)get_Imm8_Y0(bundle);
> 
> These casts are all pointless.
> 

OK, thanks. I shall change the return value of get_SrcA_Y0 and others in
opcode_tilegx.h.

[...]
>> @@ -127,8 +547,13 @@ static void decode_rrr_1_opcode_y0(struct DisasContext 
>> *dc,
>>  static void decode_rrr_5_opcode_y0(struct DisasContext *dc,
>>                                     tilegx_bundle_bits bundle)
>>  {
>> +    uint8_t rsrc = (uint8_t)get_SrcA_Y0(bundle);
>> +    uint8_t rsrcb = (uint8_t)get_SrcB_Y0(bundle);
>> +    uint8_t rdst = (uint8_t)get_Dest_Y0(bundle);
> 
> And again, and in many other places.
> 

OK, thanks.


-- 
Chen Gang

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