Re: [Qemu-devel] [PATCH 02/17] target-openrisc: Streamline arithmetic and OVE

[Bastian Koppelmann]

On 09/03/2015 02:17 AM, Richard Henderson wrote:
> +
> +void HELPER(ove)(CPUOpenRISCState *env, target_ulong test)
> +{
> +    if (unlikely(test) && (env->sr & SR_OVE)) {
> +        OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
> +        CPUState *cs = CPU(cpu);
> +
> +        cs->exception_index = EXCP_RANGE;
> +        cpu_restore_state(cs, GETPC());
> +        cpu_loop_exit(cs);
> +    }
> +}

You forgot to check the AECR register, whether the exception really occurs.

>   
> +static void gen_ove_cy(DisasContext *dc, TCGv cy)
> +{
> +    gen_helper_ove(cpu_env, cy);
> +}
> +
> +static void gen_ove_ov(DisasContext *dc, TCGv ov)
> +{
> +    gen_helper_ove(cpu_env, ov);
> +}
> +

Do we really need two functions here? They differ only by the name of 
the second argument. We should directly use gen_helper_ove ().

> +static void gen_add(DisasContext *dc, TCGv dest, TCGv srca, TCGv srcb)
> +{
> +    TCGv t0 = tcg_const_tl(0);
> +    TCGv res = tcg_temp_new();
> +    TCGv sr_cy = tcg_temp_new();
> +    TCGv sr_ov = tcg_temp_new();
> +
> +    tcg_gen_add2_tl(res, sr_cy, srca, t0, srcb, t0);
> +    tcg_gen_xor_tl(sr_ov, srca, srcb);
> +    tcg_gen_xor_tl(t0, res, srcb);
> +    tcg_gen_andc_tl(sr_ov, t0, sr_ov);
> +    tcg_temp_free(t0);
> +
> +    tcg_gen_mov_tl(dest, res);
> +    tcg_temp_free(res);
> +
> +    tcg_gen_shri_tl(sr_ov, sr_ov, TARGET_LONG_BITS - 1);
> +    tcg_gen_deposit_tl(cpu_sr, cpu_sr, sr_cy, ctz32(SR_CY), 1);
> +    tcg_gen_deposit_tl(cpu_sr, cpu_sr, sr_ov, ctz32(SR_OV), 1);
> +
> +    gen_ove_cyov(dc, sr_ov, sr_cy);
> +    tcg_temp_free(sr_ov);
> +    tcg_temp_free(sr_cy);
> +}

I do wonder, if we should use TCG globals for sr_cy and sr_ov, as you 
recommended for the TriCore target. It would not change the helper in 
case of no ovf/carry, but simplify addc. And we would not need two 
deposits for every add/sub.

> -            if (ra != 0 && rb != 0) {
> -                gen_helper_mul32(cpu_R[rd], cpu_env, cpu_R[ra], cpu_R[rb]);
> -            } else {
> -                tcg_gen_movi_tl(cpu_R[rd], 0x0);
> -            }
> +            gen_mul(dc, cpu_R[rd], cpu_R[ra], cpu_R[rb]);

What happened to this special case here? r0 should always hold zero, so 
we can keep this optimization here, or at least move it to gen_mul().

> -            if (rb != 0 && ra != 0) {
> -                TCGv_i64 result = tcg_temp_local_new_i64();
> -                TCGv_i64 tra = tcg_temp_local_new_i64();
> -                TCGv_i64 trb = tcg_temp_local_new_i64();
> -                TCGv_i64 high = tcg_temp_new_i64();
> -                TCGv_i32 sr_ove = tcg_temp_local_new_i32();
> -                TCGLabel *lab = gen_new_label();
> -                /* Calculate each result. */
> -                tcg_gen_extu_i32_i64(tra, cpu_R[ra]);
> -                tcg_gen_extu_i32_i64(trb, cpu_R[rb]);
> -                tcg_gen_mul_i64(result, tra, trb);
> -                tcg_temp_free_i64(tra);
> -                tcg_temp_free_i64(trb);
> -                tcg_gen_shri_i64(high, result, TARGET_LONG_BITS);
> -                /* Overflow or not. */
> -                tcg_gen_brcondi_i64(TCG_COND_EQ, high, 0x00000000, lab);
> -                tcg_gen_ori_tl(cpu_sr, cpu_sr, (SR_OV | SR_CY));
> -                tcg_gen_andi_tl(sr_ove, cpu_sr, SR_OVE);
> -                tcg_gen_brcondi_tl(TCG_COND_NE, sr_ove, SR_OVE, lab);
> -                gen_exception(dc, EXCP_RANGE);
> -                gen_set_label(lab);
> -                tcg_temp_free_i64(high);
> -                tcg_gen_trunc_i64_tl(cpu_R[rd], result);
> -                tcg_temp_free_i64(result);
> -                tcg_temp_free_i32(sr_ove);
> -            } else {
> -                tcg_gen_movi_tl(cpu_R[rd], 0);
> -            }
> +            gen_mulu(dc, cpu_R[rd], cpu_R[ra], cpu_R[rb]);
>               break;

The same here.

> -        {
> -            if (I16 == 0) {
> -                tcg_gen_mov_tl(cpu_R[rd], cpu_R[ra]);
> -            } else {
> -                TCGLabel *lab = gen_new_label();
> -                TCGv_i64 ta = tcg_temp_new_i64();
> -                TCGv_i64 td = tcg_temp_local_new_i64();
> -                TCGv_i32 res = tcg_temp_local_new_i32();
> -                TCGv_i32 sr_ove = tcg_temp_local_new_i32();
> -                tcg_gen_extu_i32_i64(ta, cpu_R[ra]);
> -                tcg_gen_addi_i64(td, ta, sign_extend(I16, 16));
> -                tcg_gen_extrl_i64_i32(res, td);
> -                tcg_gen_shri_i64(td, td, 32);
> -                tcg_gen_andi_i64(td, td, 0x3);
> -                /* Jump to lab when no overflow.  */
> -                tcg_gen_brcondi_i64(TCG_COND_EQ, td, 0x0, lab);
> -                tcg_gen_brcondi_i64(TCG_COND_EQ, td, 0x3, lab);
> -                tcg_gen_ori_i32(cpu_sr, cpu_sr, (SR_OV | SR_CY));
> -                tcg_gen_andi_i32(sr_ove, cpu_sr, SR_OVE);
> -                tcg_gen_brcondi_i32(TCG_COND_NE, sr_ove, SR_OVE, lab);
> -                gen_exception(dc, EXCP_RANGE);
> -                gen_set_label(lab);
> -                tcg_gen_mov_i32(cpu_R[rd], res);
> -                tcg_temp_free_i64(ta);
> -                tcg_temp_free_i64(td);
> -                tcg_temp_free_i32(res);
> -                tcg_temp_free_i32(sr_ove);
> -            }
> -        }
> +        t0 = tcg_const_tl(I16);
> +        gen_add(dc, cpu_R[rd], cpu_R[ra], t0);
> +        tcg_temp_free(t0);
>           break;

Again, why remove the special case?

>       case 0x29:    /* l.andi */
> @@ -963,13 +895,9 @@ static void dec_misc(DisasContext *dc, uint32_t insn)
>   
>       case 0x2c:    /* l.muli */
>           LOG_DIS("l.muli r%d, r%d, %d\n", rd, ra, I16);
> -        if (ra != 0 && I16 != 0) {
> -            TCGv_i32 im = tcg_const_i32(I16);
> -            gen_helper_mul32(cpu_R[rd], cpu_env, cpu_R[ra], im);
> -            tcg_temp_free_i32(im);
> -        } else {
> -            tcg_gen_movi_tl(cpu_R[rd], 0x0);
> -        }
> +        t0 = tcg_const_tl(I16);
> +        gen_mul(dc, cpu_R[rd], cpu_R[ra], t0);
> +        tcg_temp_free(t0);
Again, why remove the special case?

Cheers,
Bastian