Re: [Qemu-devel] [PATCH 04/11] target/arm: Introduce add_reg_for_lit

[Philippe Mathieu-Daudé]

On 8/7/19 6:53 AM, Richard Henderson wrote:
> Provide a common routine for the places that require ALIGN(PC, 4)
> as the base address as opposed to plain PC.  The two are always
> the same for A32, but the difference is meaningful for thumb mode.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> Note: This patch is easier to read with -b, as there are several
> large-ish indentation changes as the if statements fold together.
> ---
>  target/arm/translate-vfp.inc.c |  38 ++------
>  target/arm/translate.c         | 166 +++++++++++++++------------------
>  2 files changed, 82 insertions(+), 122 deletions(-)
> 
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 092eb5ec53..262d4177e5 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -941,14 +941,8 @@ 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);
> +    /* For thumb, use of PC is UNPREDICTABLE.  */
> +    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 +977,8 @@ 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);
> +    /* For thumb, use of PC is UNPREDICTABLE.  */
> +    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 +1017,8 @@ 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);
> -    }
> +    /* For thumb, use of PC is UNPREDICTABLE.  */
> +    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 +1095,8 @@ 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);
> -    }
> +    /* For thumb, use of PC is UNPREDICTABLE.  */
> +    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 61933865d5..71d94c053b 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -220,6 +220,23 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg)
>      return tmp;
>  }
>  
> +/*
> + * Create a new temp, REG + OFS, except PC is ALIGN(PC, 4).
> + * 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, (read_pc(s) & ~3) + ofs);
> +    } else {
> +        tcg_gen_addi_i32(tmp, cpu_R[reg], ofs);
> +    }
> +    return tmp;
> +}
> +
>  /* Set a CPU register.  The source must be a temporary and will be
>     marked as dead.  */
>  static void store_reg(DisasContext *s, int reg, TCGv_i32 var)
> @@ -9472,16 +9489,12 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                   */
>                  bool wback = extract32(insn, 21, 1);
>  
> -                if (rn == 15) {
> -                    if (insn & (1 << 21)) {
> -                        /* UNPREDICTABLE */
> -                        goto illegal_op;
> -                    }
> -                    addr = tcg_temp_new_i32();
> -                    tcg_gen_movi_i32(addr, s->pc & ~3);
> -                } else {
> -                    addr = load_reg(s, rn);
> +                if (rn == 15 && (insn & (1 << 21))) {
> +                    /* UNPREDICTABLE */
> +                    goto illegal_op;
>                  }
> +
> +                addr = add_reg_for_lit(s, rn, 0);
>                  offset = (insn & 0xff) * 4;
>                  if ((insn & (1 << 23)) == 0) {
>                      offset = -offset;
> @@ -10682,27 +10695,15 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>                          store_reg(s, rd, tmp);
>                      } else {
>                          /* Add/sub 12-bit immediate.  */
> -                        if (rn == 15) {
> -                            offset = s->pc & ~(uint32_t)3;
> -                            if (insn & (1 << 23))
> -                                offset -= imm;
> -                            else
> -                                offset += imm;
> -                            tmp = tcg_temp_new_i32();
> -                            tcg_gen_movi_i32(tmp, offset);
> -                            store_reg(s, rd, tmp);
> +                        if (insn & (1 << 23)) {
> +                            imm = -imm;

:)

> +                        }
> +                        tmp = add_reg_for_lit(s, rn, imm);
> +                        if (rn == 13 && rd == 13) {
> +                            /* ADD SP, SP, imm or SUB SP, SP, imm */
> +                            store_sp_checked(s, tmp);
>                          } else {
> -                            tmp = load_reg(s, rn);
> -                            if (insn & (1 << 23))
> -                                tcg_gen_subi_i32(tmp, tmp, imm);
> -                            else
> -                                tcg_gen_addi_i32(tmp, tmp, imm);
> -                            if (rn == 13 && rd == 13) {
> -                                /* ADD SP, SP, imm or SUB SP, SP, imm */
> -                                store_sp_checked(s, tmp);
> -                            } else {
> -                                store_reg(s, rd, tmp);
> -                            }
> +                            store_reg(s, rd, tmp);
>                          }
>                      }
>                  }
> @@ -10816,61 +10817,53 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>              }
>          }
>          memidx = get_mem_index(s);
> -        if (rn == 15) {
> -            addr = tcg_temp_new_i32();
> -            /* PC relative.  */
> -            /* s->pc has already been incremented by 4.  */
> -            imm = s->pc & 0xfffffffc;
> -            if (insn & (1 << 23))
> -                imm += insn & 0xfff;
> -            else
> -                imm -= insn & 0xfff;
> -            tcg_gen_movi_i32(addr, imm);
> +        imm = insn & 0xfff;
> +        if (insn & (1 << 23)) {
> +            /* PC relative or Positive offset.  */
> +            addr = add_reg_for_lit(s, rn, imm);
> +        } else if (rn == 15) {
> +            /* PC relative with negative offset.  */
> +            addr = add_reg_for_lit(s, rn, -imm);
>          } else {
>              addr = load_reg(s, rn);
> -            if (insn & (1 << 23)) {
> -                /* Positive offset.  */
> -                imm = insn & 0xfff;
> -                tcg_gen_addi_i32(addr, addr, imm);
> -            } else {
> -                imm = insn & 0xff;
> -                switch ((insn >> 8) & 0xf) {
> -                case 0x0: /* Shifted Register.  */
> -                    shift = (insn >> 4) & 0xf;
> -                    if (shift > 3) {
> -                        tcg_temp_free_i32(addr);
> -                        goto illegal_op;
> -                    }
> -                    tmp = load_reg(s, rm);
> -                    if (shift)

'diff -b' shows me you removed this 'if (shift)' but ...

> -                        tcg_gen_shli_i32(tmp, tmp, shift);
> -                    tcg_gen_add_i32(addr, addr, tmp);
> -                    tcg_temp_free_i32(tmp);
> -                    break;
> -                case 0xc: /* Negative offset.  */
> -                    tcg_gen_addi_i32(addr, addr, -imm);
> -                    break;
> -                case 0xe: /* User privilege.  */
> -                    tcg_gen_addi_i32(addr, addr, imm);
> -                    memidx = get_a32_user_mem_index(s);
> -                    break;
> -                case 0x9: /* Post-decrement.  */
> -                    imm = -imm;
> -                    /* Fall through.  */
> -                case 0xb: /* Post-increment.  */
> -                    postinc = 1;
> -                    writeback = 1;
> -                    break;
> -                case 0xd: /* Pre-decrement.  */
> -                    imm = -imm;
> -                    /* Fall through.  */
> -                case 0xf: /* Pre-increment.  */
> -                    writeback = 1;
> -                    break;
> -                default:
> +            imm = insn & 0xff;
> +            switch ((insn >> 8) & 0xf) {
> +            case 0x0: /* Shifted Register.  */
> +                shift = (insn >> 4) & 0xf;
> +                if (shift > 3) {
>                      tcg_temp_free_i32(addr);
>                      goto illegal_op;
>                  }
> +                tmp = load_reg(s, rm);
> +                if (shift) {

... I'm seeing it here. Odd.

> +                    tcg_gen_shli_i32(tmp, tmp, shift);
> +                }
> +                tcg_gen_add_i32(addr, addr, tmp);
> +                tcg_temp_free_i32(tmp);
> +                break;
> +            case 0xc: /* Negative offset.  */
> +                tcg_gen_addi_i32(addr, addr, -imm);
> +                break;
> +            case 0xe: /* User privilege.  */
> +                tcg_gen_addi_i32(addr, addr, imm);
> +                memidx = get_a32_user_mem_index(s);
> +                break;
> +            case 0x9: /* Post-decrement.  */
> +                imm = -imm;
> +                /* Fall through.  */
> +            case 0xb: /* Post-increment.  */
> +                postinc = 1;
> +                writeback = 1;
> +                break;
> +            case 0xd: /* Pre-decrement.  */
> +                imm = -imm;
> +                /* Fall through.  */
> +            case 0xf: /* Pre-increment.  */
> +                writeback = 1;
> +                break;
> +            default:
> +                tcg_temp_free_i32(addr);
> +                goto illegal_op;
>              }
>          }
>  
> @@ -11066,10 +11059,7 @@ static void disas_thumb_insn(DisasContext *s, 
> uint32_t insn)
>          if (insn & (1 << 11)) {
>              rd = (insn >> 8) & 7;
>              /* load pc-relative.  Bit 1 of PC is ignored.  */
> -            val = read_pc(s) + ((insn & 0xff) * 4);
> -            val &= ~(uint32_t)2;
> -            addr = tcg_temp_new_i32();
> -            tcg_gen_movi_i32(addr, val);
> +            addr = add_reg_for_lit(s, 15, (insn & 0xff) * 4);
>              tmp = tcg_temp_new_i32();
>              gen_aa32_ld32u_iss(s, tmp, addr, get_mem_index(s),
>                                 rd | ISSIs16Bit);
> @@ -11447,16 +11437,8 @@ static void disas_thumb_insn(DisasContext *s, 
> uint32_t insn)
>           *  - Add PC/SP (immediate)
>           */
>          rd = (insn >> 8) & 7;
> -        if (insn & (1 << 11)) {
> -            /* SP */
> -            tmp = load_reg(s, 13);
> -        } else {
> -            /* PC. bit 1 is ignored.  */
> -            tmp = tcg_temp_new_i32();
> -            tcg_gen_movi_i32(tmp, read_pc(s) & ~(uint32_t)2);
> -        }
>          val = (insn & 0xff) * 4;
> -        tcg_gen_addi_i32(tmp, tmp, val);
> +        tmp = add_reg_for_lit(s, insn & (1 << 11) ? 13 : 15, val);
>          store_reg(s, rd, tmp);
>          break;

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>