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

[Richard Henderson <rth@twiddle.net>]

> +static inline void gen_add_d(TCGv ret, TCGv r1, TCGv r2)
> +{
> +    TCGv t0 = tcg_temp_new_i32();
> +    /* Addition and set V/SV bits */
> +    tcg_gen_add_tl(ret, r1, r2);
> +    /* calc V bit */
> +    tcg_gen_xor_tl(cpu_PSW_V, ret, r1);
> +    tcg_gen_xor_tl(t0, r1, r2);
> +    tcg_gen_andc_tl(cpu_PSW_V, cpu_PSW_V, t0);

I'd prefer not to see a v5 until you've actually tested some of this.  If that
requires that you implement some of the 32-bit instructions, so be it.

You cannot overwrite any of the inputs until you've computed overflow.  Since
you're passing the cpu_gpr_d variables directly, which means that any insn that
computes a = a + b overwrites r1 before you've used it in either xor.

> +static inline void gen_cond_add(int cond, TCGv r1, TCGv r2, TCGv r3,
> +                                TCGv r4)

The type of cond should be TCGCond.

> +    /* Calc PSW_V */
> +    tcg_gen_xor_tl(temp, temp, r1);
> +    tcg_gen_xor_tl(temp, r1, r2);
> +    tcg_gen_andc_tl(temp2, temp, t0);
> +    tcg_gen_movcond_tl(cond, cpu_PSW_V, r4, t0, temp2, cpu_PSW_V);
> +    /* Set PSW_SV */
> +    tcg_gen_or_tl(cpu_PSW_SV, temp2, cpu_PSW_SV);
> +    /* calc AV bit */
> +    tcg_gen_add_tl(temp2, temp2, temp);
> +    tcg_gen_xor_tl(temp2, temp2, temp);
> +    tcg_gen_movcond_tl(cond, cpu_PSW_AV, r4, t0, temp2, cpu_PSW_AV);
> +    /* calc SAV bit */
> +    tcg_gen_or_tl(cpu_PSW_SAV, temp2, cpu_PSW_SAV);

The sticky bits still need a movcond.  Or create a setcond mask like

	mask = r4 cond 0
	mask = mask << 31
	temp2 &= mask
	PSW_SV |= temp2;
	...
	temp2 &= mask
	PSW_SAV |= temp2

> +static void gen_shaci(TCGv ret, TCGv r1, int32_t shift_count)
> +{
> +    uint32_t msk, msk_start;
> +    TCGv temp = tcg_temp_new();
> +    TCGv temp2 = tcg_temp_new();
> +    TCGv t_max = tcg_const_i32(0x7FFFFFFF >> shift_count);
> +    TCGv t_min = tcg_const_i32(-(0x80000000L) >> shift_count);

These constants are only used in the shift_count > 0 case, and indeed cannot be
computed except within the shift_count > 0 case without provoking underined
behaviour.

> +    if (shift_count == 0) {
> +        /* Clear PSW.C */
> +        tcg_gen_movi_tl(cpu_PSW_C, 0);
> +        tcg_gen_mov_tl(ret, r1);

Also clear V.

> +    } else if (shift_count == 32) {
> +        /* fill ret completly with sign bit */
> +        tcg_gen_sari_tl(ret, r1, 31);

Should be shift_count == -32; also clear V.

> +    } else if (shift_count > 0) {
> +        tcg_gen_shli_tl(ret, r1, shift_count);
> +        /* calc carry */
> +        msk_start = 32 - shift_count;
> +        msk = ((1 << shift_count) - 1) << msk_start;
> +        tcg_gen_andi_tl(cpu_PSW_C, r1, msk);
> +        /* calc v/sv bits */
> +        tcg_gen_setcond_tl(TCG_COND_GT, temp, r1, t_max);
> +        tcg_gen_setcond_tl(TCG_COND_LT, temp2, r1, t_min);
> +        tcg_gen_or_tl(cpu_PSW_V, temp, temp2);
> +        /* calc sv */
> +        tcg_gen_or_tl(cpu_PSW_SV, cpu_PSW_V, cpu_PSW_SV);

You must compute C and V before overwriting r1.

> +    } else {
> +        tcg_gen_sari_tl(ret, r1, -(shift_count));
> +        /* calc carry */
> +        msk = (1 << (shift_count - 1)) - 1;
> +        tcg_gen_andi_tl(cpu_PSW_C, r1, msk);

Likewise, and clear V.


r~