Re: [Qemu-devel] [PATCH 1/6 v6] target-tilegx: Firstly add TILE-Gx with minimized features

[Richard Henderson]

On 03/18/2015 09:34 AM, Chen Gang wrote:
> +static void gen_fnop(void)
> +{
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "(f)nop\n");
> +}
> +
> +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);
> +}

Wow, this is a lot more than before.  Good progress.

> +/*
> + * 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)
> +{
> +    TCGv mask = tcg_temp_new_i64();
> +    TCGv tmp = dest_gr(dc, rdst);
> +
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "bfextu r%d, r%d, %d, %d\n",
> +                  rdst, rsrc, start, end);
> +
> +    tcg_gen_movi_i64(tmp, -1ULL);
> +    tcg_gen_movi_i64(mask, end);
> +
> +    tcg_gen_subi_i64(mask, mask, start);
> +    tcg_gen_andi_i64(mask, mask, 63);
> +    tcg_gen_shl_i64(mask, tmp, mask);
> +    tcg_gen_shli_i64(mask, mask, 1);
> +    tcg_gen_xori_i64(mask, mask, -1ULL);

This computation of MASK is only dependent on START and END, which are known at
translation time.  Thus you should perform this computation at translation time
here in C, rather than defer the computation with TCG opcodes.

> +    tcg_gen_rotli_i64(tmp, load_gr(dc, rsrc), start);
> +    tcg_gen_and_i64(tmp, tmp, mask);

Which then makes this tcg_gen_andi_i64.

> +static void gen_mulx(struct DisasContext *dc,
> +                     uint8_t rdst, uint8_t rsrc, uint8_t rsrcb)
> +{
> +    TCGv tmp = tcg_temp_new_i64();
> +    TCGv vdst = dest_gr(dc, rdst);
> +
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "mulx r%d, r%d, r%d\n", rdst, rsrc, 
> rsrcb);
> +
> +    tcg_gen_ext32s_i64(vdst, load_gr(dc, rsrc));
> +    tcg_gen_ext32s_i64(tmp, load_gr(dc, rsrcb));
> +
> +    tcg_gen_mul_i64(tmp, vdst, tmp);
> +    tcg_gen_ext32s_i64(vdst, tmp);

Note that you don't need the extensions prior to the MUL.  The high 32-bits of
the inputs don't affect the low 32-bits of the product.

> +static void gen_shl16insli(struct DisasContext *dc,
> +                           uint8_t rdst, uint8_t rsrc, uint16_t uimm16)
> +{
> +    TCGv vdst = dest_gr(dc, rdst);
> +
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "shl16insli r%d, r%d, %llx\n",
> +                  rdst, rsrc, (long long)uimm16);

Do not cast to long long.  Print with just %x.

> +static int gen_beqz(struct DisasContext *dc, uint8_t rsrc, int32_t off)
> +{
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "beqz(t) r%d, %d\n", rsrc, off);
> +
> +    dc->jmp.dest = tcg_temp_new_i64();
> +    dc->jmp.val1 = tcg_temp_new_i64();
> +    dc->jmp.val2 = tcg_temp_new_i64();
> +
> +    dc->jmp.cond = TCG_COND_EQ;
> +    tcg_gen_movi_i64(dc->jmp.dest,
> +                     dc->pc + (int64_t)off * TILEGX_BUNDLE_SIZE_IN_BYTES);

It will be helpful for the disassembly if you print the destination of the
branch rather than the offset.

> +    qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_opcode_y0, %16.16llx\n", bundle);

You shouldn't use "llx".  You need to use PRIx64 instead.  This will enable
proper operation on Windows.

This patch set is just about ready for approval (though I don't imagine it will
be merged until the qemu 2.4 development cycle).


r~