On 10 January 2014 18:05, Richard Henderson <rth@xxxxxxxxxxx> wrote:
> On 01/10/2014 09:12 AM, Peter Maydell wrote:
>> + TCGMemOp memop = MO_TE + size;
>
> Double space after =. Multiple occurrences.
Just this one plus its copy-n-paste in do_vec_st, I think.
>> + if (is_postidx) {
>> + int rm = extract32(insn, 16, 5);
>> + if (rm == 31) {
>> + tcg_gen_mov_i64(cpu_reg_sp(s, rn), tcg_addr);
>> + } else {
>> + tcg_gen_add_i64(cpu_reg_sp(s, rn), cpu_reg(s, rn), cpu_reg(s, rm));
>> + }
>
> Second cpu_reg must be cpu_reg_sp as well.
Yes. Unfortunately the testing tool we're using doesn't
support testing of SP-relative accesses, so this kind
of bug can slip through.
> Maybe better to hoist load of
> tcg_rn to before initial assignment of tcg_addr?
Not sure what you have in mind here. Pulling the
cpu_reg_sp() call out one level like:
if (is_postidx) {
int rm = extract32(insn, 16, 5);
TCGv_i64 tcg_rn = cpu_reg_sp(s, rn);
if (rm == 31) {
tcg_gen_mov_i64(tcg_rn, tcg_addr);
} else {
tcg_gen_add_i64(tcg_rn, tcg_rn, cpu_reg(s, rm));
}
}
seems like a good idea though.
thanks
-- PMM
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
