On Oct 22, 2009, at 08:40, Riihimaki Juha (Nokia-D/Helsinki) 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?
To make it more clear what I'm after, look at the patch below that
changes the code into what I think is correct functionality. The patch
applies on top of the v2 resource leak patch which I just sent a short
while ago.
Cheers,
Juha
---
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 813f661..7598246 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4696,18 +4696,12 @@ static int disas_neon_data_insn(CPUState *
env, DisasContext *s, uint32_t insn)
else
gen_neon_narrow_satu(size - 1, tmp,
cpu_V0);
}
- if (pass == 0) {
- if (size != 3) {
- dead_tmp(tmp2);
- }
- tmp2 = tmp;
- } else {
- neon_store_reg(rd, 0, tmp2);
- neon_store_reg(rd, 1, tmp);
- }
+ neon_store_reg(rd, pass, tmp);
} /* for pass */
if (size == 3) {
tcg_temp_free_i64(tmp64);
+ } else {
+ dead_tmp(tmp2);
}
} else if (op == 10) {
/* VSHLL */
