On 7/29/19 7:15 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:50, Richard Henderson
> <richard.hender...@linaro.org> 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 <richard.hender...@linaro.org>
>> ---
>> 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~
