Re: [Qemu-devel] [PATCH 07/67] target/arm: Introduce add_reg_for_lit

[Peter Maydell]

On Fri, 26 Jul 2019 at 18:50, Richard Henderson
<address@hidden> wrote:
>
> Used only on the thumb side so far, but will be more obvious
> once we start unifying the implementation of A32+T32.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate-vfp.inc.c |  34 +------
>  target/arm/translate.c         | 163 +++++++++++++++------------------
>  2 files changed, 76 insertions(+), 121 deletions(-)
>
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index e7389bc057..4066b2febf 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -941,14 +941,7 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, 
> arg_VLDR_VSTR_sp *a)
>          offset = -offset;
>      }
>
> -    if (s->thumb && a->rn == 15) {
> -        /* This is actually UNPREDICTABLE */
> -        addr = tcg_temp_new_i32();
> -        tcg_gen_movi_i32(addr, s->pc & ~2);
> -    } else {
> -        addr = load_reg(s, a->rn);
> -    }
> -    tcg_gen_addi_i32(addr, addr, offset);
> +    addr = add_reg_for_lit(s, a->rn, offset);
>      tmp = tcg_temp_new_i32();
>      if (a->l) {
>          gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
> @@ -983,14 +976,7 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, 
> arg_VLDR_VSTR_dp *a)
>          offset = -offset;
>      }
>
> -    if (s->thumb && a->rn == 15) {
> -        /* This is actually UNPREDICTABLE */
> -        addr = tcg_temp_new_i32();
> -        tcg_gen_movi_i32(addr, s->pc & ~2);
> -    } else {
> -        addr = load_reg(s, a->rn);
> -    }
> -    tcg_gen_addi_i32(addr, addr, offset);
> +    addr = add_reg_for_lit(s, a->rn, offset);
>      tmp = tcg_temp_new_i64();
>      if (a->l) {
>          gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
> @@ -1029,13 +1015,7 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, 
> arg_VLDM_VSTM_sp *a)
>          return true;
>      }
>
> -    if (s->thumb && a->rn == 15) {
> -        /* This is actually UNPREDICTABLE */
> -        addr = tcg_temp_new_i32();
> -        tcg_gen_movi_i32(addr, s->pc & ~2);
> -    } else {
> -        addr = load_reg(s, a->rn);
> -    }
> +    addr = add_reg_for_lit(s, a->rn, 0);
>      if (a->p) {
>          /* pre-decrement */
>          tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> @@ -1112,13 +1092,7 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, 
> arg_VLDM_VSTM_dp *a)
>          return true;
>      }
>
> -    if (s->thumb && a->rn == 15) {
> -        /* This is actually UNPREDICTABLE */
> -        addr = tcg_temp_new_i32();
> -        tcg_gen_movi_i32(addr, s->pc & ~2);
> -    } else {
> -        addr = load_reg(s, a->rn);
> -    }
> +    addr = add_reg_for_lit(s, a->rn, 0);
>      if (a->p) {
>          /* pre-decrement */
>          tcg_gen_addi_i32(addr, addr, -(a->imm << 2));
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index a48e9a90f8..5e2dd8bb16 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -214,6 +214,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg)
>      return tmp;
>  }
>
> +/*
> + * Create a new temp, incremented by OFS, except PC is aligned but not
> + * incremented for thumb.  This is used for load/store for which use of
> + * PC implies (literal), or ADD that implies ADR.
> + */
> +static TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
> +{
> +    TCGv_i32 tmp = tcg_temp_new_i32();
> +
> +    if (reg == 15) {
> +        tcg_gen_movi_i32(tmp, (s->pc_read & ~3) + ofs);
> +    } else {
> +        tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
> +    }
> +    return tmp;
> +}

This is losing the information in the comments about the UNPREDICTABLE
cases. Are there callsites where the new function is called where
"thumb and reg == 15" is not UNPREDICTABLE, or are they all
that way?

thanks
-- PMM