Re: [Qemu-arm] [PATCH 15/67] target/arm: Convert Saturating addition and subtraction

[Peter Maydell]

On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<address@hidden> wrote:
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper.h    |  1 -
>  target/arm/op_helper.c | 15 ---------
>  target/arm/translate.c | 74 +++++++++++++++++++++++++++---------------
>  target/arm/a32.decode  | 10 ++++++
>  target/arm/t32.decode  |  9 +++++
>  5 files changed, 66 insertions(+), 43 deletions(-)
>
> +/*
> + * Saturating addition and subtraction
> + */
> +
> +static bool op_qaddsub(DisasContext *s, arg_rrr *a, bool add, bool doub)
> +{
> +    TCGv_i32 t0, t1;
> +
> +    if (s->thumb
> +        ? !arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)
> +        : !ENABLE_ARCH_5TE) {
> +        return false;
> +    }
> +
> +    t0 = load_reg(s, a->rm);
> +    t1 = load_reg(s, a->rn);
> +    if (doub) {
> +        gen_helper_add_saturate(t1, cpu_env, t1, t1);
> +    }
> +    if (add) {
> +        gen_helper_add_saturate(t0, cpu_env, t0, t1);
> +    } else {
> +        gen_helper_sub_saturate(t0, cpu_env, t0, t1);
> +    }
> +    tcg_temp_free_i32(t1);
> +    store_reg(s, a->rd, t0);
> +    return true;
> +}
> +

> -        case 0x5: /* saturating add/subtract */
> -            ARCH(5TE);
> -            rd = (insn >> 12) & 0xf;
> -            rn = (insn >> 16) & 0xf;
> -            tmp = load_reg(s, rm);
> -            tmp2 = load_reg(s, rn);
> -            if (op1 & 2)
> -                gen_helper_double_saturate(tmp2, cpu_env, tmp2);
> -            if (op1 & 1)
> -                gen_helper_sub_saturate(tmp, cpu_env, tmp, tmp2);
> -            else
> -                gen_helper_add_saturate(tmp, cpu_env, tmp, tmp2);
> -            tcg_temp_free_i32(tmp2);
> -            store_reg(s, rd, tmp);
> -            break;

This is changing the way we generate code in the middle
of also doing the refactoring. Could you not do this,
please (or where it really does make sense to do it then
call it out in the commit message)? It makes it harder
to review because now I have to read the patch for two
different changes at once...

thanks
-- PMM