Re: [Qemu-devel] [PATCH v2 06/15] target-tricore: Add instructions of SRC opcode format

[Richard Henderson <rth@twiddle.net>]

On 07/14/2014 10:41 AM, Bastian Koppelmann wrote:
> +target_ulong helper_shac(CPUTRICOREState *env, target_ulong r1,
> +                         target_ulong r2, target_ulong len)
> +{
> +    target_ulong carry_out, msk, msk_start, msk_len, ret;
> +    int32_t shift_count;
> +    int const6;
> +    const6 = sextract32(r2, 0, len);
> +
> +    if (const6 >= 0) {
> +        if (const6 != 0) {
> +            msk_start = 32 - const6;
> +            msk_len = 31-msk_start;
> +            msk = ((1 << msk_len) - 1) << msk_start;
> +            carry_out = ((r1 & msk) != 0);
> +        } else {
> +            carry_out = 0;
> +        }
> +        ret = r1 << const6;
> +    } else {
> +
> +        shift_count = 0 - const6;
> +        ret = (int32_t)r1 >> shift_count;
> +        msk = (1 << (shift_count - 1)) - 1;
> +        carry_out = ((r1 & msk) != 0);
> +    }
> +    if (carry_out) {
> +        /* TODO: carry out */
> +    }
> +    return ret;
> +}

Why a helper for SHA?  It's not any more difficult than SH.

> +static void gen_shi(TCGv ret, TCGv r1, int32_t r2, int len)
> +{
> +/* shift_count = sign_ext(const4[3:0]);
> +   D[a] = (shift_count >= 0) ? D[a] << shift_count : D[a] >> (-shift_count); */
> +    int shift_count = sextract32(r2, 0, len);

Careful with your documentation: you're adding the 16-bit documentation as
opposed to the more generic 32-bit documentation.

I do not think you should have a "len" parameter at all.  We've already
sign-extended const4 in decode_src_opc, so there's no need to do it again.

> +static void gen_shaci(TCGv ret, TCGv r1, int32_t con, int len)
> +{
> +    TCGv temp = tcg_const_i32(con);
> +
> +    gen_shac(ret, r1, temp, len);
> +
> +    tcg_temp_free(temp);
> +}

In particular, SHACI with an immediate is pretty much exactly SHI except with
an arithmetic right shift instead of a logical right shift.

(Yes, there's some carry and overflow bit computation to do, but it's not like
you've implemented any of that in your current implementation either.)

> +
> +/*
> + * Functions for decoding instructions
> + */
> +
> +static void decode_src_opc(DisasContext *ctx, int op1)
> +{
> +    int r1;
> +    int32_t const4;
> +    TCGv temp, temp2;
> +
> +    r1 = MASK_OP_SRC_S1D(ctx->opcode);
> +    const4 = MASK_OP_SRC_CONST4_SEXT(ctx->opcode);
> +
> +    switch (op1) {
> +
> +    case OPC1_16_SRC_ADD:

Watch the silly blank likes, above the case.  And the end-of-file blank lines
in some of the other patches.

> +        tcg_gen_addi_tl(cpu_gpr_d[r1], cpu_gpr_d[r1], const4);

Are you planning to come back to implement V and AV bits later?

> +    case OPC1_16_SRC_MOV_A:
> +        /* load const4 again unsigned */
> +        const4 = MASK_OP_SRC_CONST4(ctx->opcode);
> +        tcg_gen_movi_tl(cpu_gpr_a[r1], const4);

Err.. I don't think this is right.  I see "signed" on page 3-224.

> +    case OPC1_16_SRC_SHA:
> +        /* FIXME: const too long */
> +        gen_shaci(cpu_gpr_d[r1], cpu_gpr_d[r1], const4, 4);
> +        break;

Huh?  Why the fixme?


r~