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

[Richard Henderson]

On 7/29/19 7:15 AM, Peter Maydell wrote:
> 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?

These call sites are that way, but this function will eventually be used for
LDR (literal) and ADR, which obviously are not UNPREDICTABLE.

I don't think this comment attached to this code is useful as-is.  Either we do
the natural a32-ish behaviour and use ALIGN(PC,4), or we should
gen_illegal_op() and be done with it.

Would you prefer a function like

/* Use of PC is UNPREDICTABLE in thumb mode, but allowed in arm mode. */
static TCGv_i32 load_reg_nothumbpc(DisasContext *s, int reg)
{
    if (unlikely(reg == 15) && s->thumb) {
        gen_illegal_op(s);
        /* Unreachable tcg ops will be deleted but must still be legal. */
        return tcg_const_i32(0);
    }
    return load_reg(s, reg);
}

for these specific usages?


r~