Thanks for your feedback. I'll iron out the issues you mentioned,
please look at my further inlined comments below.
Cheers,
Juha
On Oct 21, 2009, at 22:20, ext Laurent Desnogues wrote:
>> @@ -4676,12 +4694,16 @@ static int disas_neon_data_insn(CPUState *
>> env, DisasContext *s, uint32_t insn)
>> gen_neon_narrow_satu(size - 1, tmp,
>> cpu_V0);
>> }
>> if (pass == 0) {
>> + dead_tmp(tmp2);
>
> This looks wrong if size == 3 since we have TCGV_UNUSED(tmp2).
You're right. However, looking at the surrounding code a bit closer I
began to wonder if it works correctly at all since tmp2 is used as a
shift value if size < 2 but it is also used to store the result of the
first pass. Therefore the result of the first pass is used as a shift
value during second pass... perhaps not correct? Wouldn't it make more
sense to store the result of the first pass in the lower half of the
destination neon register directly during the first pass? I see no
point in keeping it in a temporary variable and only store both halves
of the destination neon register during the second pass. Especially
since there is no memory access involved, everything is done in
registers. What do you think?
>> @@ -5511,8 +5534,9 @@ static int disas_neon_data_insn(CPUState * env,
>> DisasContext *s, uint32_t insn)
>> tcg_gen_movi_i32(tmp, 0);
>> }
>> tmp2 = neon_load_reg(rm, 0);
>> - gen_helper_neon_tbl(tmp2, tmp2, tmp, tcg_const_i32
>> (rn),
>> - tcg_const_i32(n));
>> + TCGv tmp_rn = tcg_const_i32(rn);
>> + TCGv tmp_n = tcg_const_i32(n);
>
> I don't know what QEMU coding rules say about declarations in
> the middle of the code, but I don't like them too much :-)
Ah, sorry. I forgot to clean up my own cleanups ;) I'll fix this and
the other instances you noticed.
>> @@ -5671,6 +5696,7 @@ static void gen_storeq_reg(DisasContext *s, int
>> rlow, int rhigh, TCGv_i64 val)
>> tcg_gen_shri_i64(val, val, 32);
>> tcg_gen_trunc_i64_i32(tmp, val);
>> store_reg(s, rhigh, tmp);
>> + tcg_temp_free_i64(val);
>
> I'd prefer to see val freed after calls to gen_storeq_reg even
> if it means a longer patch. The current situation with regards
> to implicit temp new and free is already messy enough.
Certainly. I guess the whole file could be patched to remove all such
implicit allocations and deallocations and make everything explicit;
that should clear things up and make it more readable. I could do that
as well. I implemented this change to make it behave like the other
store_reg functions in the target-arm/translate.c, I thought having
methods with similar functionality behaving differently in regards to
the arguments would make things even messier.
>> @@ -7673,6 +7719,7 @@ static int disas_thumb2_insn(CPUState *env,
>> DisasContext *s, uint16_t insn_hw1)
>> tmp = load_reg(s, rn);
>> addr = tcg_const_i32(insn & 0xff);
>> gen_helper_v7m_msr(cpu_env, addr, tmp);
>> + tcg_temp_free_i32(addr);
>
> Shouldn't tmp be freed too?
Correct. I'll add that.
