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

[Chen Gang]

On 3/21/15 01:45, Richard Henderson wrote:
> On 03/20/2015 08:25 AM, Chen Gang wrote:
>> +/*
>> + * 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);
>> +
>> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "bfextu r%d, r%d, %d, %d\n",
>> +                  rdst, rsrc, start, end);
>> +
>> +    tcg_gen_rotli_i64(tmp, load_gr(dc, rsrc), start);
> 
> Wrong direction rotate: rotri.
> 

OK, thank you very much!!


>> +static void decode_rrr_1_opcode_y0(struct DisasContext *dc,
>> +                                   tilegx_bundle_bits bundle)
>> +{
>> +    switch (get_RRROpcodeExtension_Y0(bundle)) {
>> +    case UNARY_RRR_1_OPCODE_Y0:
>> +        switch (get_UnaryOpcodeExtension_Y0(bundle)) {
>> +        case NOP_UNARY_OPCODE_Y0:
>> +        case  FNOP_UNARY_OPCODE_Y0:
>> +            if (!get_SrcA_Y0(bundle) && !get_Dest_Y0(bundle)) {
>> +                gen_fnop();
>> +                return;
>> +            }
>> +            break;
>> +        case CNTLZ_UNARY_OPCODE_Y0:
>> +        case CNTTZ_UNARY_OPCODE_Y0:
>> +        case FSINGLE_PACK1_UNARY_OPCODE_Y0:
>> +        case PCNT_UNARY_OPCODE_Y0:
>> +        case REVBITS_UNARY_OPCODE_Y0:
>> +        case REVBYTES_UNARY_OPCODE_Y0:
>> +        case TBLIDXB0_UNARY_OPCODE_Y0:
>> +        case TBLIDXB1_UNARY_OPCODE_Y0:
>> +        case TBLIDXB2_UNARY_OPCODE_Y0:
>> +        case TBLIDXB3_UNARY_OPCODE_Y0:
>> +        default:
>> +            break;
>> +        }
>> +        break;
>> +    case SHL1ADD_RRR_1_OPCODE_Y0:
>> +    case SHL2ADD_RRR_1_OPCODE_Y0:
>> +    case SHL3ADD_RRR_1_OPCODE_Y0:
>> +    case RRR_1_OPCODE_Y0:
> 
> RRR_1_OPCODE_Y0 doesn't belong.  It's the main opcode that brought us here.
> 

OK, thanks.

>> +    default:
>> +        break;
>> +    }
>> +
>> +    qemu_log_mask(LOG_UNIMP, "UNIMP rrr_1_opcode_y0, [" TARGET_FMT_lx "]\n",
>> +                  (uint64_t)bundle);
> 
> Eh, TARGET_FMT_lx is tied to TARGET_LONG_BITS, and you're not using 
> target_long
> but uint64_t.  I think it would be better to use "%016" PRIx64 directly, or
> create your own macro for this file.
> 

Oh, really, 'bundle' is defined as 'tilegx_bundle_bits' which is not for
"target long" (in 32-bit tilepro, 'tilepro_bundle_bits' is also 64-bit).

I shall define FMT64X for it within 'translate.c', just like:

  tests/tcg/test-i386.c:53:#define FMT64X "%016" PRIx64


Thanks.
-- 
Chen Gang

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