Re: [Qemu-devel] [PATCH 12/12] target-arm: A64: add support for compare and branch imm

[Peter Maydell]

On 4 December 2013 00:10, Richard Henderson <address@hidden> wrote:
> On 12/04/2013 10:51 AM, Peter Maydell wrote:
>> @@ -184,6 +184,18 @@ static TCGv_i64 cpu_reg(DisasContext *s, int reg)
>>      }
>>  }
>>
>> +/* read a cpu register in 32bit/64bit mode to dst */
>> +static void read_cpu_reg(DisasContext *s, TCGv_i64 dst, int reg, int sf)
>> +{
>> +    if (reg == 31) {
>> +        tcg_gen_movi_i64(dst, 0);
>> +    } else if (sf) {
>> +        tcg_gen_mov_i64(dst, cpu_X[reg]);
>> +    } else { /* (!sf) */
>> +        tcg_gen_ext32u_i64(dst, cpu_X[reg]);
>> +    }
>> +}
>
> I think this should be more like cpu_reg and return a TCGv instead of force a
> copy into a newly generated temporary.  You've got a pool of auto-freeing
> temporaries, after all.

You're right that we can just make this function return the TCGv
temp rather than making the caller pass one in. Are you suggesting
the 64-bit case should return cpu_X[reg] rather than a copy of it,
though? I think it would be pretty hard to reason about if you had to
remember that sometimes you got a trashable copy and sometimes
the real register.

(I'm still a bit on the fence about that pool of auto-freeing temporaries.
Manual temp management is certainly a fertile source of decoder bugs,
but in the longer term we might want to push the functionality down into
the common code rather than having an ad-hoc thing in one decoder.)

>> +    if (op) { /* CBNZ */
>> +        tcg_gen_brcondi_i64(TCG_COND_EQ, tcg_cmp, 0, label_nomatch);
>> +    } else { /* CBZ */
>> +        tcg_gen_brcondi_i64(TCG_COND_NE, tcg_cmp, 0, label_nomatch);
>> +    }
>
> Similar comments to TBNZ/TBZ apply here.

Agreed.

thanks
-- PMM