Re: [Qemu-devel] [PATCH 01/10] target-arm: A64: Add SIMD ld/st multiple

[Peter Maydell]

On 10 January 2014 18:05, Richard Henderson <address@hidden> 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