target/ppc: fix vbpermd in big endian hosts

[PATCH] target/ppc: fix vbpermd in big endian hosts

Posted by matheus.ferst@eldorado.org.br 3 years, 8 months ago

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

The extract64 arguments are not endian dependent as they are only used
for bitwise operations. The current behavior in little-endian hosts is
correct; since the indexes in VRB are in PowerISA-ordering, we should
always invert the value before calling extract64. Also, using the VsrD
macro, we can have a single EXTRACT_BIT definition for big and
little-endian with the correct behavior.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The
complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX,
but it will need more testing. For now, we're just changing what is
necessary to fix the instruction.
---
 target/ppc/int_helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index 105b626d1b..4c5d3f03f8 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8)
 #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)])
 #define VBPERMD_INDEX(i) (i)
 #define VBPERMQ_DW(index) (((index) & 0x40) != 0)
-#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1))
 #else
 #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)])
 #define VBPERMD_INDEX(i) (1 - i)
 #define VBPERMQ_DW(index) (((index) & 0x40) == 0)
-#define EXTRACT_BIT(avr, i, index) \
-        (extract64((avr)->u64[1 - i], 63 - index, 1))
 #endif
+#define EXTRACT_BIT(avr, i, index) \
+        (extract64((avr)->VsrD(i), 63 - index, 1))
 
 void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
 {
-- 
2.25.1

Re: [PATCH] target/ppc: fix vbpermd in big endian hosts

Posted by Daniel Henrique Barboza 3 years, 8 months ago

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

On 6/1/22 09:53, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> The extract64 arguments are not endian dependent as they are only used
> for bitwise operations. The current behavior in little-endian hosts is
> correct; since the indexes in VRB are in PowerISA-ordering, we should
> always invert the value before calling extract64. Also, using the VsrD
> macro, we can have a single EXTRACT_BIT definition for big and
> little-endian with the correct behavior.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The
> complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX,
> but it will need more testing. For now, we're just changing what is
> necessary to fix the instruction.
> ---
>   target/ppc/int_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index 105b626d1b..4c5d3f03f8 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8)
>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)])
>   #define VBPERMD_INDEX(i) (i)
>   #define VBPERMQ_DW(index) (((index) & 0x40) != 0)
> -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1))
>   #else
>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)])
>   #define VBPERMD_INDEX(i) (1 - i)
>   #define VBPERMQ_DW(index) (((index) & 0x40) == 0)
> -#define EXTRACT_BIT(avr, i, index) \
> -        (extract64((avr)->u64[1 - i], 63 - index, 1))
>   #endif
> +#define EXTRACT_BIT(avr, i, index) \
> +        (extract64((avr)->VsrD(i), 63 - index, 1))
>   
>   void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>   {

Re: [PATCH] target/ppc: fix vbpermd in big endian hosts

Posted by Richard Henderson 3 years, 8 months ago

On 6/1/22 05:53, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> The extract64 arguments are not endian dependent as they are only used
> for bitwise operations. The current behavior in little-endian hosts is
> correct; since the indexes in VRB are in PowerISA-ordering, we should
> always invert the value before calling extract64. Also, using the VsrD
> macro, we can have a single EXTRACT_BIT definition for big and
> little-endian with the correct behavior.
> 
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
> Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The
> complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX,
> but it will need more testing. For now, we're just changing what is
> necessary to fix the instruction.
> ---
>   target/ppc/int_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] target/ppc: fix vbpermd in big endian hosts

Posted by Philippe Mathieu-Daudé via 3 years, 8 months ago

+Mark for commit ef96e3ae96.

On 1/6/22 14:53, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> The extract64 arguments are not endian dependent as they are only used
> for bitwise operations. The current behavior in little-endian hosts is
> correct; since the indexes in VRB are in PowerISA-ordering, we should
> always invert the value before calling extract64. Also, using the VsrD
> macro, we can have a single EXTRACT_BIT definition for big and
> little-endian with the correct behavior.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The
> complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX,
> but it will need more testing. For now, we're just changing what is
> necessary to fix the instruction.
> ---
>   target/ppc/int_helper.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
> index 105b626d1b..4c5d3f03f8 100644
> --- a/target/ppc/int_helper.c
> +++ b/target/ppc/int_helper.c
> @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8)
>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)])
>   #define VBPERMD_INDEX(i) (i)
>   #define VBPERMQ_DW(index) (((index) & 0x40) != 0)
> -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1))
>   #else
>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)])
>   #define VBPERMD_INDEX(i) (1 - i)
>   #define VBPERMQ_DW(index) (((index) & 0x40) == 0)
> -#define EXTRACT_BIT(avr, i, index) \
> -        (extract64((avr)->u64[1 - i], 63 - index, 1))
>   #endif
> +#define EXTRACT_BIT(avr, i, index) \
> +        (extract64((avr)->VsrD(i), 63 - index, 1))
>   
>   void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>   {

Re: [PATCH] target/ppc: fix vbpermd in big endian hosts

Posted by Mark Cave-Ayland 3 years, 8 months ago

On 01/06/2022 15:21, Philippe Mathieu-Daudé via wrote:

> +Mark for commit ef96e3ae96.
> 
> On 1/6/22 14:53, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> The extract64 arguments are not endian dependent as they are only used
>> for bitwise operations. The current behavior in little-endian hosts is
>> correct; since the indexes in VRB are in PowerISA-ordering, we should
>> always invert the value before calling extract64. Also, using the VsrD
>> macro, we can have a single EXTRACT_BIT definition for big and
>> little-endian with the correct behavior.
>>
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>> Found this bug while refactoring VECTOR_FOR_INORDER_I uses. The
>> complete patch series will also use Vsr[DB] instead of VBPERM[DQ]_INDEX,
>> but it will need more testing. For now, we're just changing what is
>> necessary to fix the instruction.
>> ---
>>   target/ppc/int_helper.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
>> index 105b626d1b..4c5d3f03f8 100644
>> --- a/target/ppc/int_helper.c
>> +++ b/target/ppc/int_helper.c
>> @@ -1307,14 +1307,13 @@ XXGENPCV(XXGENPCVDM, 8)
>>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[(i)])
>>   #define VBPERMD_INDEX(i) (i)
>>   #define VBPERMQ_DW(index) (((index) & 0x40) != 0)
>> -#define EXTRACT_BIT(avr, i, index) (extract64((avr)->u64[i], index, 1))
>>   #else
>>   #define VBPERMQ_INDEX(avr, i) ((avr)->u8[15 - (i)])
>>   #define VBPERMD_INDEX(i) (1 - i)
>>   #define VBPERMQ_DW(index) (((index) & 0x40) == 0)
>> -#define EXTRACT_BIT(avr, i, index) \
>> -        (extract64((avr)->u64[1 - i], 63 - index, 1))
>>   #endif
>> +#define EXTRACT_BIT(avr, i, index) \
>> +        (extract64((avr)->VsrD(i), 63 - index, 1))
>>   void helper_vbpermd(ppc_avr_t *r, ppc_avr_t *a, ppc_avr_t *b)
>>   {

I'm not too familiar with vbpermd, however in general the use of the VsrX() macros is 
the right way to ensure things work correctly on both big-endian and little-endian 
hosts, so it looks fine to me.

FWIW with all the great improvements being done in this area, I think that Matheus 
and Daniel have picked things up really quickly and have a much better test setup 
than the G4 Mac Mini I used to do the original gvec work. If I happen to spot 
something on the mailing list then I'll likely reply, but otherwise I'm happy to 
allow things to progress without requiring an explicit Ack from me (these days my 
testing is mostly confined to checking that MacOS 9/X boot okay).


ATB,

Mark.