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 <richard.hender...@linaro.org>
> ---
> 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é <phi...@redhat.com>
