* [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
@ 2009-10-21 10:17 Juha.Riihimaki
2009-10-21 10:46 ` Laurent Desnogues
0 siblings, 1 reply; 6+ messages in thread
From: Juha.Riihimaki @ 2009-10-21 10:17 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]
Shift immediate value is incorrectly overwritten by a temporary
variable in the processing of NEON vsri, vshl and vsli instructions.
Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
---
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 59bf7bc..c92ecc6 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4094,7 +4094,7 @@ static int disas_neon_data_insn(CPUState * env,
DisasContext *s, uint32_t insn)
int pairwise;
int u;
int n;
- uint32_t imm;
+ uint32_t imm, imm2;
TCGv tmp;
TCGv tmp2;
TCGv tmp3;
@@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
env, DisasContext *s, uint32_t insn)
switch (size) {
case 0:
if (op == 4)
- imm = 0xff >> -shift;
+ imm2 = 0xff >> -shift;
else
- imm = (uint8_t)(0xff << shift);
- imm |= imm << 8;
- imm |= imm << 16;
+ imm2 = (uint8_t)(0xff << shift);
+ imm2 |= imm2 << 8;
+ imm2 |= imm2 << 16;
break;
case 1:
if (op == 4)
- imm = 0xffff >> -shift;
+ imm2 = 0xffff >> -shift;
else
- imm = (uint16_t)(0xffff << shift);
- imm |= imm << 16;
+ imm2 = (uint16_t)(0xffff << shift);
+ imm2 |= imm2 << 16;
break;
case 2:
if (op == 4)
- imm = 0xffffffffu >> -shift;
+ imm2 = 0xffffffffu >> -shift;
else
- imm = 0xffffffffu << shift;
+ imm2 = 0xffffffffu << shift;
break;
default:
abort();
}
tmp2 = neon_load_reg(rd, pass);
- tcg_gen_andi_i32(tmp, tmp, imm);
- tcg_gen_andi_i32(tmp2, tmp2, ~imm);
+ tcg_gen_andi_i32(tmp, tmp, imm2);
+ tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
tcg_gen_or_i32(tmp, tmp, tmp2);
dead_tmp(tmp2);
}
[-- Attachment #2: translate.c.neonimm.diff --]
[-- Type: application/octet-stream, Size: 2734 bytes --]
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 59bf7bc..c92ecc6 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4094,7 +4094,7 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
int pairwise;
int u;
int n;
- uint32_t imm;
+ uint32_t imm, imm2;
TCGv tmp;
TCGv tmp2;
TCGv tmp3;
@@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
switch (size) {
case 0:
if (op == 4)
- imm = 0xff >> -shift;
+ imm2 = 0xff >> -shift;
else
- imm = (uint8_t)(0xff << shift);
- imm |= imm << 8;
- imm |= imm << 16;
+ imm2 = (uint8_t)(0xff << shift);
+ imm2 |= imm2 << 8;
+ imm2 |= imm2 << 16;
break;
case 1:
if (op == 4)
- imm = 0xffff >> -shift;
+ imm2 = 0xffff >> -shift;
else
- imm = (uint16_t)(0xffff << shift);
- imm |= imm << 16;
+ imm2 = (uint16_t)(0xffff << shift);
+ imm2 |= imm2 << 16;
break;
case 2:
if (op == 4)
- imm = 0xffffffffu >> -shift;
+ imm2 = 0xffffffffu >> -shift;
else
- imm = 0xffffffffu << shift;
+ imm2 = 0xffffffffu << shift;
break;
default:
abort();
}
tmp2 = neon_load_reg(rd, pass);
- tcg_gen_andi_i32(tmp, tmp, imm);
- tcg_gen_andi_i32(tmp2, tmp2, ~imm);
+ tcg_gen_andi_i32(tmp, tmp, imm2);
+ tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
tcg_gen_or_i32(tmp, tmp, tmp2);
dead_tmp(tmp2);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
2009-10-21 10:17 [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops Juha.Riihimaki
@ 2009-10-21 10:46 ` Laurent Desnogues
2009-10-22 6:49 ` Juha.Riihimaki
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Desnogues @ 2009-10-21 10:46 UTC (permalink / raw)
To: Juha.Riihimaki; +Cc: qemu-devel
On Wed, Oct 21, 2009 at 12:17 PM, <Juha.Riihimaki@nokia.com> wrote:
> Shift immediate value is incorrectly overwritten by a temporary
> variable in the processing of NEON vsri, vshl and vsli instructions.
>
> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> ---
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 59bf7bc..c92ecc6 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4094,7 +4094,7 @@ static int disas_neon_data_insn(CPUState * env,
> DisasContext *s, uint32_t insn)
> int pairwise;
> int u;
> int n;
> - uint32_t imm;
> + uint32_t imm, imm2;
I would prefer mask to imm2, but that's personal taste :-)
> TCGv tmp;
> TCGv tmp2;
> TCGv tmp3;
> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
> env, DisasContext *s, uint32_t insn)
> switch (size) {
> case 0:
> if (op == 4)
> - imm = 0xff >> -shift;
> + imm2 = 0xff >> -shift;
> else
> - imm = (uint8_t)(0xff << shift);
> - imm |= imm << 8;
> - imm |= imm << 16;
> + imm2 = (uint8_t)(0xff << shift);
> + imm2 |= imm2 << 8;
> + imm2 |= imm2 << 16;
> break;
> case 1:
> if (op == 4)
> - imm = 0xffff >> -shift;
> + imm2 = 0xffff >> -shift;
> else
> - imm = (uint16_t)(0xffff << shift);
> - imm |= imm << 16;
> + imm2 = (uint16_t)(0xffff << shift);
> + imm2 |= imm2 << 16;
> break;
> case 2:
> if (op == 4)
> - imm = 0xffffffffu >> -shift;
> + imm2 = 0xffffffffu >> -shift;
> else
> - imm = 0xffffffffu << shift;
> + imm2 = 0xffffffffu << shift;
> break;
> default:
> abort();
> }
> tmp2 = neon_load_reg(rd, pass);
> - tcg_gen_andi_i32(tmp, tmp, imm);
> - tcg_gen_andi_i32(tmp2, tmp2, ~imm);
> + tcg_gen_andi_i32(tmp, tmp, imm2);
> + tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
> tcg_gen_or_i32(tmp, tmp, tmp2);
> dead_tmp(tmp2);
> }
I mostly agree with this, except there's a mistake already
present in the original code: for a size of 2 the shift amount
can be 32 (look at VSRI in the ARM Architecture Reference
Manual). Note in this case, shift will be -32.
Laurent
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
2009-10-21 10:46 ` Laurent Desnogues
@ 2009-10-22 6:49 ` Juha.Riihimaki
2009-10-22 7:18 ` Laurent Desnogues
0 siblings, 1 reply; 6+ messages in thread
From: Juha.Riihimaki @ 2009-10-22 6:49 UTC (permalink / raw)
To: laurent.desnogues; +Cc: qemu-devel
On Oct 21, 2009, at 13:46, ext Laurent Desnogues wrote:
>> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
>> env, DisasContext *s, uint32_t insn)
>> switch (size) {
>> case 0:
>> if (op == 4)
>> - imm = 0xff >> -shift;
>> + imm2 = 0xff >> -shift;
>> else
>> - imm = (uint8_t)(0xff << shift);
>> - imm |= imm << 8;
>> - imm |= imm << 16;
>> + imm2 = (uint8_t)(0xff << shift);
>> + imm2 |= imm2 << 8;
>> + imm2 |= imm2 << 16;
>> break;
>> case 1:
>> if (op == 4)
>> - imm = 0xffff >> -shift;
>> + imm2 = 0xffff >> -shift;
>> else
>> - imm = (uint16_t)(0xffff <<
>> shift);
>> - imm |= imm << 16;
>> + imm2 = (uint16_t)(0xffff <<
>> shift);
>> + imm2 |= imm2 << 16;
>> break;
>> case 2:
>> if (op == 4)
>> - imm = 0xffffffffu >> -shift;
>> + imm2 = 0xffffffffu >> -shift;
>> else
>> - imm = 0xffffffffu << shift;
>> + imm2 = 0xffffffffu << shift;
>> break;
>> default:
>> abort();
>> }
>> tmp2 = neon_load_reg(rd, pass);
>> - tcg_gen_andi_i32(tmp, tmp, imm);
>> - tcg_gen_andi_i32(tmp2, tmp2, ~imm);
>> + tcg_gen_andi_i32(tmp, tmp, imm2);
>> + tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
>> tcg_gen_or_i32(tmp, tmp, tmp2);
>> dead_tmp(tmp2);
>> }
>
> I mostly agree with this, except there's a mistake already
> present in the original code: for a size of 2 the shift amount
> can be 32 (look at VSRI in the ARM Architecture Reference
> Manual). Note in this case, shift will be -32.
True. However, I'm not sure if this causes incorrect behavior or not.
I'm not a NEON expert but how I understand the VSRI instruction is
that it will shift the element value before it is inserted in the
destination vector, therefore with element size 2 and shift 32 the
result would be always zero and I guess that would still apply if the
shift was -32: does it matter in which direction you shift if you
anyway shift all the bits out?
Regards,
Juha
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
2009-10-22 6:49 ` Juha.Riihimaki
@ 2009-10-22 7:18 ` Laurent Desnogues
2009-10-22 7:33 ` Juha.Riihimaki
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Desnogues @ 2009-10-22 7:18 UTC (permalink / raw)
To: Juha.Riihimaki; +Cc: qemu-devel
On Thu, Oct 22, 2009 at 8:49 AM, <Juha.Riihimaki@nokia.com> wrote:
>
> On Oct 21, 2009, at 13:46, ext Laurent Desnogues wrote:
>
>>> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
>>> env, DisasContext *s, uint32_t insn)
>>> switch (size) {
>>> case 0:
>>> if (op == 4)
>>> - imm = 0xff >> -shift;
>>> + imm2 = 0xff >> -shift;
>>> else
>>> - imm = (uint8_t)(0xff << shift);
>>> - imm |= imm << 8;
>>> - imm |= imm << 16;
>>> + imm2 = (uint8_t)(0xff << shift);
>>> + imm2 |= imm2 << 8;
>>> + imm2 |= imm2 << 16;
>>> break;
>>> case 1:
>>> if (op == 4)
>>> - imm = 0xffff >> -shift;
>>> + imm2 = 0xffff >> -shift;
>>> else
>>> - imm = (uint16_t)(0xffff <<
>>> shift);
>>> - imm |= imm << 16;
>>> + imm2 = (uint16_t)(0xffff <<
>>> shift);
>>> + imm2 |= imm2 << 16;
>>> break;
>>> case 2:
>>> if (op == 4)
>>> - imm = 0xffffffffu >> -shift;
>>> + imm2 = 0xffffffffu >> -shift;
>>> else
>>> - imm = 0xffffffffu << shift;
>>> + imm2 = 0xffffffffu << shift;
>>> break;
>>> default:
>>> abort();
>>> }
>>> tmp2 = neon_load_reg(rd, pass);
>>> - tcg_gen_andi_i32(tmp, tmp, imm);
>>> - tcg_gen_andi_i32(tmp2, tmp2, ~imm);
>>> + tcg_gen_andi_i32(tmp, tmp, imm2);
>>> + tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
>>> tcg_gen_or_i32(tmp, tmp, tmp2);
>>> dead_tmp(tmp2);
>>> }
>>
>> I mostly agree with this, except there's a mistake already
>> present in the original code: for a size of 2 the shift amount
>> can be 32 (look at VSRI in the ARM Architecture Reference
>> Manual). Note in this case, shift will be -32.
>
> True. However, I'm not sure if this causes incorrect behavior or not.
> I'm not a NEON expert but how I understand the VSRI instruction is
> that it will shift the element value before it is inserted in the
> destination vector, therefore with element size 2 and shift 32 the
> result would be always zero and I guess that would still apply if the
> shift was -32: does it matter in which direction you shift if you
> anyway shift all the bits out?
The problem is not in the NEON semantics but rather the C
one: the behaviour of a shift by an amount greater than or
equal to the bit-width of the type is undefined; in this case
imm2 is 32-bit and the shift is 32-bit. What you want is
imm2 = 0 if shift is -32, as you correctly guessed.
Laurent
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
2009-10-22 7:18 ` Laurent Desnogues
@ 2009-10-22 7:33 ` Juha.Riihimaki
2009-10-22 7:40 ` Laurent Desnogues
0 siblings, 1 reply; 6+ messages in thread
From: Juha.Riihimaki @ 2009-10-22 7:33 UTC (permalink / raw)
To: laurent.desnogues; +Cc: qemu-devel
On Oct 22, 2009, at 10:18, ext Laurent Desnogues wrote:
> On Thu, Oct 22, 2009 at 8:49 AM, <Juha.Riihimaki@nokia.com> wrote:
>>
>> On Oct 21, 2009, at 13:46, ext Laurent Desnogues wrote:
>>
>>>> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
>>>> env, DisasContext *s, uint32_t insn)
>>>> switch (size) {
>>>> case 0:
>>>> if (op == 4)
>>>> - imm = 0xff >> -shift;
>>>> + imm2 = 0xff >> -shift;
>>>> else
>>>> - imm = (uint8_t)(0xff <<
>>>> shift);
>>>> - imm |= imm << 8;
>>>> - imm |= imm << 16;
>>>> + imm2 = (uint8_t)(0xff <<
>>>> shift);
>>>> + imm2 |= imm2 << 8;
>>>> + imm2 |= imm2 << 16;
>>>> break;
>>>> case 1:
>>>> if (op == 4)
>>>> - imm = 0xffff >> -shift;
>>>> + imm2 = 0xffff >> -shift;
>>>> else
>>>> - imm = (uint16_t)(0xffff <<
>>>> shift);
>>>> - imm |= imm << 16;
>>>> + imm2 = (uint16_t)(0xffff <<
>>>> shift);
>>>> + imm2 |= imm2 << 16;
>>>> break;
>>>> case 2:
>>>> if (op == 4)
>>>> - imm = 0xffffffffu >> -shift;
>>>> + imm2 = 0xffffffffu >> -shift;
>>>> else
>>>> - imm = 0xffffffffu << shift;
>>>> + imm2 = 0xffffffffu << shift;
>>>> break;
>>>> default:
>>>> abort();
>>>> }
>>>> tmp2 = neon_load_reg(rd, pass);
>>>> - tcg_gen_andi_i32(tmp, tmp, imm);
>>>> - tcg_gen_andi_i32(tmp2, tmp2, ~imm);
>>>> + tcg_gen_andi_i32(tmp, tmp, imm2);
>>>> + tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
>>>> tcg_gen_or_i32(tmp, tmp, tmp2);
>>>> dead_tmp(tmp2);
>>>> }
>>>
>>> I mostly agree with this, except there's a mistake already
>>> present in the original code: for a size of 2 the shift amount
>>> can be 32 (look at VSRI in the ARM Architecture Reference
>>> Manual). Note in this case, shift will be -32.
>>
>> True. However, I'm not sure if this causes incorrect behavior or not.
>> I'm not a NEON expert but how I understand the VSRI instruction is
>> that it will shift the element value before it is inserted in the
>> destination vector, therefore with element size 2 and shift 32 the
>> result would be always zero and I guess that would still apply if the
>> shift was -32: does it matter in which direction you shift if you
>> anyway shift all the bits out?
>
> The problem is not in the NEON semantics but rather the C
> one: the behaviour of a shift by an amount greater than or
> equal to the bit-width of the type is undefined; in this case
> imm2 is 32-bit and the shift is 32-bit. What you want is
> imm2 = 0 if shift is -32, as you correctly guessed.
Ah, I see. Doesn't the same issue apply for the 8bit and 16bit element
sizes as well? I suppose it will be sufficient to check for a negative
shift value and in that case force the mask value to zero.
Regards,
Juha
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops
2009-10-22 7:33 ` Juha.Riihimaki
@ 2009-10-22 7:40 ` Laurent Desnogues
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Desnogues @ 2009-10-22 7:40 UTC (permalink / raw)
To: Juha.Riihimaki; +Cc: qemu-devel
On Thu, Oct 22, 2009 at 9:33 AM, <Juha.Riihimaki@nokia.com> wrote:
>
> On Oct 22, 2009, at 10:18, ext Laurent Desnogues wrote:
>
>> On Thu, Oct 22, 2009 at 8:49 AM, <Juha.Riihimaki@nokia.com> wrote:
>>>
>>> On Oct 21, 2009, at 13:46, ext Laurent Desnogues wrote:
>>>
>>>>> @@ -4624,31 +4624,31 @@ static int disas_neon_data_insn(CPUState *
>>>>> env, DisasContext *s, uint32_t insn)
>>>>> switch (size) {
>>>>> case 0:
>>>>> if (op == 4)
>>>>> - imm = 0xff >> -shift;
>>>>> + imm2 = 0xff >> -shift;
>>>>> else
>>>>> - imm = (uint8_t)(0xff <<
>>>>> shift);
>>>>> - imm |= imm << 8;
>>>>> - imm |= imm << 16;
>>>>> + imm2 = (uint8_t)(0xff <<
>>>>> shift);
>>>>> + imm2 |= imm2 << 8;
>>>>> + imm2 |= imm2 << 16;
>>>>> break;
>>>>> case 1:
>>>>> if (op == 4)
>>>>> - imm = 0xffff >> -shift;
>>>>> + imm2 = 0xffff >> -shift;
>>>>> else
>>>>> - imm = (uint16_t)(0xffff <<
>>>>> shift);
>>>>> - imm |= imm << 16;
>>>>> + imm2 = (uint16_t)(0xffff <<
>>>>> shift);
>>>>> + imm2 |= imm2 << 16;
>>>>> break;
>>>>> case 2:
>>>>> if (op == 4)
>>>>> - imm = 0xffffffffu >> -shift;
>>>>> + imm2 = 0xffffffffu >> -shift;
>>>>> else
>>>>> - imm = 0xffffffffu << shift;
>>>>> + imm2 = 0xffffffffu << shift;
>>>>> break;
>>>>> default:
>>>>> abort();
>>>>> }
>>>>> tmp2 = neon_load_reg(rd, pass);
>>>>> - tcg_gen_andi_i32(tmp, tmp, imm);
>>>>> - tcg_gen_andi_i32(tmp2, tmp2, ~imm);
>>>>> + tcg_gen_andi_i32(tmp, tmp, imm2);
>>>>> + tcg_gen_andi_i32(tmp2, tmp2, ~imm2);
>>>>> tcg_gen_or_i32(tmp, tmp, tmp2);
>>>>> dead_tmp(tmp2);
>>>>> }
>>>>
>>>> I mostly agree with this, except there's a mistake already
>>>> present in the original code: for a size of 2 the shift amount
>>>> can be 32 (look at VSRI in the ARM Architecture Reference
>>>> Manual). Note in this case, shift will be -32.
>>>
>>> True. However, I'm not sure if this causes incorrect behavior or not.
>>> I'm not a NEON expert but how I understand the VSRI instruction is
>>> that it will shift the element value before it is inserted in the
>>> destination vector, therefore with element size 2 and shift 32 the
>>> result would be always zero and I guess that would still apply if the
>>> shift was -32: does it matter in which direction you shift if you
>>> anyway shift all the bits out?
>>
>> The problem is not in the NEON semantics but rather the C
>> one: the behaviour of a shift by an amount greater than or
>> equal to the bit-width of the type is undefined; in this case
>> imm2 is 32-bit and the shift is 32-bit. What you want is
>> imm2 = 0 if shift is -32, as you correctly guessed.
>
> Ah, I see. Doesn't the same issue apply for the 8bit and 16bit element
> sizes as well? I suppose it will be sufficient to check for a negative
> shift value and in that case force the mask value to zero.
For 8- and 16-bit elements, your shift will always be <= 16 (in
absolute value I mean :-), so you have no issue for these
cases.
Laurent
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-10-22 7:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-21 10:17 [Qemu-devel] [PATCH 07/12] target-arm: fix neon vsri, vshl and vsli ops Juha.Riihimaki
2009-10-21 10:46 ` Laurent Desnogues
2009-10-22 6:49 ` Juha.Riihimaki
2009-10-22 7:18 ` Laurent Desnogues
2009-10-22 7:33 ` Juha.Riihimaki
2009-10-22 7:40 ` Laurent Desnogues
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).